Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WaferMap component - basic storybook implementation #834

Merged
merged 17 commits into from
Nov 29, 2022

Conversation

DStavilaNI
Copy link
Contributor

@DStavilaNI DStavilaNI commented Nov 24, 2022

WaferMap component - basic storybook implementation

🤨 Rationale

This PR implements an initial configuration for the story to be used for the wafermap component. The PR also adds some basic functionality and example code for both the wafermap class and its corresponding template.

👩‍💻 Implementation

  • Implemented the corresponding story code
  • Implemented some example data sets to be used for testing purposes within the story
  • Implemented example code (within the component) and the template to showcase different usages

🧪 Testing

  • Local testing has been done

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

}
},
maxCharacters: {
description:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a name: configuration that uses the attribute name for these args, ie name: 'max-characters'. Similar to the above it allows it to map more closely to the HTML representation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applied

Copy link
Member

@rajsite rajsite Nov 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DStavilaNI I think there was a misunderstanding. The goal is so the names in the story align with the names in the user html example:
image

Currently the html that is rendered is also incorrect:

image

I'd expect the attribute names in storybook to look like max-characters as my comment described and not maxcharacters as your change shows. For such a big deviation from comments it may be worth asking for a clarification.

The expected HTML is:

<nimble-wafer-map
    colors-scale-mode="linear"
    die-labels-hidden
    max-characters="4"
    orientation="left"
    quadrant="bottom-left"
></nimble-wafer-map>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of revertting the changes in the PR I created the following issue: #836

Those changes need to be addressed for others to build on the storybook changes.

@DStavilaNI DStavilaNI changed the title Users/dstavila/wafermap story WaferMap component - basic storybook implementation Nov 29, 2022
@DStavilaNI DStavilaNI merged commit 001156d into main Nov 29, 2022
@DStavilaNI DStavilaNI deleted the users/dstavila/wafermap-story branch November 29, 2022 17:32
@rajsite rajsite mentioned this pull request Nov 29, 2022
4 tasks
returnedValue = highLightedValueSets[0];
break;
case 'set2':
returnedValue = highLightedValueSets[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here need to be "1" instead of "0"

case 'set2': returnedValue = highLightedValueSets[1];

}
return returnedValue;
}}
:highlightedValues="${x => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here maybe you need to have 4 cases because in the "sets.ts" file you have 4 examples.

returnedValue = wafermapDieSets[2];
break;
case 'set4':
returnedValue = wafermapDieSets[2];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here maybe you need to have only 2 cases because in the "sets.ts" file you have only 2 examples.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also case 3 and 4 has the same data "wafermapDieSets[2]", maybe it should be updated too, if we want to have 4 cases :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants