Update label in SingleSelect and MultiSelect #2354
Conversation
🦋 Changeset detectedLatest commit: bb10fdf The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +50 B (+0.05%) Total Size: 101 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-ucfxvxcrev.chromatic.com/ Chromatic results:
|
daniellewhyte
changed the title
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (b1f69be) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2354"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2354 |
daniellewhyte
changed the title
|
@Khan/wonder-blocks This is now ready for review. |
There was a problem hiding this comment.
This is looking solid to me but I'd be curious to hear what @jandrade and @beaesguerra have to say. These components are pretty complex!
Great job, @daniellewhyte!
| * one option is selected and `labelAsText` is undefined. | ||
| * **Note** this is only supported for SingleSelect and MultiSelect, not Combobox. | ||
| */ | ||
| export const CustomOptionItemsWithoutTextLabel: StoryComponentType = { |
There was a problem hiding this comment.
Maybe it's just me, but I found the "without text label" part of these new stories a little confusing! 😅
Would something like "with node label" or "with child element as label" reflect the unique aspect of these updates?
There was a problem hiding this comment.
Yes, good suggestion! just pushed an update
| * `labelAsText` is undefined. | ||
| * **Note** this is only supported for SingleSelect and MultiSelect, not Combobox. | ||
| */ | ||
| export const CustomOptionItemWithoutTextLabel: StoryComponentType = { |
There was a problem hiding this comment.
Same as above. Would renaming these stories with something like "With element child as label" work for you? It would make it easier to understand them when scanning through Storybook.
| @@ -150,7 +150,7 @@ describe("MultiSelect", () => { | |||
|
|
|||
| // Assert | |||
| expect(await screen.findByRole("button")).toHaveTextContent( | |||
| "1 student", | |||
There was a problem hiding this comment.
Wow, that's a pretty confusing test result looking at this older code. Where did "student" even come from? Your change seems way more intuitive.
There was a problem hiding this comment.
I was wondering the same thing! Looks like "1 student" was coming from the defaultLabels that are passed in!
There was a problem hiding this comment.
Bea's assumption is correct. This is part of what Danielle fixed with this PR. Previously, we used the label associated for that, but from now on we'll be using whatever it is passed in as a custom option item label. This means that if we don't use a custom label, then someSelected(1) should be used in this case. @daniellewhyte I'm not sure if there's an existing unit test for this case, but if not, it would be good adding one here to prevent possible regressions in the future.
| value={String(index)} | ||
| horizontalRule="full-width" | ||
| label={ | ||
| <span> |
There was a problem hiding this comment.
This tested well in VoiceOver! I couldn't get my Windows VM to load the local Storybook environment for NVDA but I'm pretty sure it will be fine.
There was a problem hiding this comment.
Thanks for looking into this, @daniellewhyte 😄
I noticed a bug when we set isFilterable in the CustomOptionItemsWithNodeLabel story and had follow up questions!
| @@ -150,7 +150,7 @@ describe("MultiSelect", () => { | |||
|
|
|||
| // Assert | |||
| expect(await screen.findByRole("button")).toHaveTextContent( | |||
| "1 student", | |||
There was a problem hiding this comment.
I was wondering the same thing! Looks like "1 student" was coming from the defaultLabels that are passed in!
| }; | ||
|
|
||
| return ( | ||
| <MultiSelect |
There was a problem hiding this comment.
I played around with the changes and found if we set isFilterable in this story, the filtering functionality doesn't work as expected! However, if we add a labelAsText prop to the OptionItem components with the content (ie. the locale in this example), filtering works as expected.
I'm wondering, should we use labelAsText prop when we use a node label? And if so, would that solve for the original issue where the select opener is labeled with an empty string? Let me know though if there's some context I'm missing!
cc: @jandrade , I think he was running into similar things when he worked on the combobox!
There was a problem hiding this comment.
That's a good callout, and the proposed solution does solve the original issue.
There was a problem hiding this comment.
@beaesguerra Actually I don't think this works. If you add labelAsText to the option item, you see plain text as the label for the menu.
That said, my specific use case does not use filtering.
There was a problem hiding this comment.
Ah I see, we want to include the icon in the field! Thanks for trying that out!
For other use cases that would need filtering, the labelAsText prop would fix the filtering issue (with the drawback that the menu opener would show the labelAsText instead of the custom option). With that said, the changes look good to me, though I would also like to get @jandrade's thoughts since he has more context around this!
There was a problem hiding this comment.
I understand that the specific use case in this PR doesn't need filtering, but I worry that adding this feature while introducing a known issue might be counterproductive if there's another call site that needs both things (custom options + filtering).
I'd really recommend applying the fix/change in this PR before landing this change.
I was thinking that it might be solved by changing the filter logic to use .toString() after the getSelectOpenerLabel invocation. I'm not fully sure if this is the right solution but it might be worth trying it out for both MultiSelect and SingleSelect.
| @@ -150,7 +150,7 @@ describe("MultiSelect", () => { | |||
|
|
|||
| // Assert | |||
| expect(await screen.findByRole("button")).toHaveTextContent( | |||
| "1 student", | |||
There was a problem hiding this comment.
Bea's assumption is correct. This is part of what Danielle fixed with this PR. Previously, we used the label associated for that, but from now on we'll be using whatever it is passed in as a custom option item label. This means that if we don't use a custom label, then someSelected(1) should be used in this case. @daniellewhyte I'm not sure if there's an existing unit test for this case, but if not, it would be good adding one here to prevent possible regressions in the future.
| }; | ||
|
|
||
| return ( | ||
| <MultiSelect |
There was a problem hiding this comment.
I understand that the specific use case in this PR doesn't need filtering, but I worry that adding this feature while introducing a known issue might be counterproductive if there's another call site that needs both things (custom options + filtering).
I'd really recommend applying the fix/change in this PR before landing this change.
I was thinking that it might be solved by changing the filter logic to use .toString() after the getSelectOpenerLabel invocation. I'm not fully sure if this is the right solution but it might be worth trying it out for both MultiSelect and SingleSelect.
|
@Khan/wonder-blocks I'm going to go ahead and close this. For anyone who stumbles upon this in the future, context is here: https://khanacademy.slack.com/archives/C4PE1QM5Y/p1731532324639189 |
Summary:
When you pass in a JSX Element as a label to
OptionItem, the SelectOpener is labeled with an empty string. This PR updates SelectOpener in theSingleSelectandMultiSelectcomponents to return the JSX as a label in that case.This change is being made to unblock supporting TEX in the Perseus Dropdown widget.
Issue: LIT-1425
Test plan: