-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
change/@ni-nimble-components-297ab22e-342a-48cb-94f9-a5d8e7fe9bfa.json
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/tests/wafer-map.stories.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/tests/wafer-map.stories.ts
Outdated
Show resolved
Hide resolved
change/@ni-nimble-components-297ab22e-342a-48cb-94f9-a5d8e7fe9bfa.json
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/tests/wafer-map.stories.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/tests/wafer-map.stories.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/tests/wafer-map.stories.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/tests/wafer-map.stories.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/tests/wafer-map.stories.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/tests/wafer-map.stories.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/tests/wafer-map.stories.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/tests/wafer-map.stories.ts
Outdated
Show resolved
Hide resolved
} | ||
}, | ||
maxCharacters: { | ||
description: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied
There was a problem hiding this comment.
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:
Currently the html that is rendered is also incorrect:
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>
There was a problem hiding this comment.
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.
returnedValue = highLightedValueSets[0]; | ||
break; | ||
case 'set2': | ||
returnedValue = highLightedValueSets[0]; |
There was a problem hiding this comment.
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 => { |
There was a problem hiding this comment.
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]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
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
🧪 Testing
✅ Checklist