WB-1779: Add startIcon prop to Combobox #2364
Conversation
🦋 Changeset detectedLatest commit: 76790a9 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 |
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 (b047234) 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="PR2364"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2364 |
|
Size Change: +153 B (+0.15%) Total Size: 101 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-eejlmdwnxz.chromatic.com/ Chromatic results:
|
There was a problem hiding this comment.
Changes look good, I noticed the prop docs needed to be updated though!
I also have some questions/things we can consider (doesn't have to be part of this PR!):
-
I was also working on a similar magnifying icon in the SearchField component in PR SearchField: Add validation props and refine states #2363! I noticed the styling in the code component looked a bit different from the Figma component so I updated the styles while I was there 😄 . Since both Combobox and SearchField use TextField, it would be great if we could consolidate how the start icon gets rendered! There were several considerations for styling the icon like the
disabledandlightprops. One idea on how we could consolidate the styling would be to add astartIconprop toTextFieldand it could provide the color, sizing, spacing. This could be useful forTextFieldon its own too! What do you think?- I was also wondering if a
endIconprop would also be useful for things like the Chevron icon, though it is an icon button so maybe the prop should support icon buttons too. Another use case would be the clear icon button in SearchField, or a show/hide icon button on a password field! The Combobox can potentially have 2 end icon buttons though for the clear and expand buttons, so perhaps it would take a React.Node instead of limiting it to Icon/IconButton!
- I was also wondering if a
-
For the combobox, would it be helpful for users to always have the magnifying glass icon there by default so there is a hint that they can type to search? Then teams can customize the icon for specific use cases if needed!
| @@ -132,6 +133,14 @@ type Props = { | |||
| */ | |||
| // TODO(WB-1740): Add support to `inline` and `both` values. | |||
| autoComplete?: "none" | "list" | undefined; | |||
|
|
|||
| /** | |||
| * The icon (wrapped by a button) that allows opening the listbox widget | |||
There was a problem hiding this comment.
I think the prop docs need to be updated for startIcon!
There was a problem hiding this comment.
oh oops, changing it! (I copy pasted from the impl. spec so I'm going to update it there as well.
There was a problem hiding this comment.
Fixed! btw I also modified the new story to include both the default and disabled state.
I'm thinking about creating an All Variants story to capture all the Combobox styles, but I'd do it in a separate PR. wdyt?
@beaesguerra Good points, here are my thoughts:
1.b. The original goal was to include
|
👍 That simplifies things, great!
1.b. Oops I should've clarified! I was thinking about an
Got it, thanks for the context! Something else I'm thinking about is if there are things we can do to make sure icon usage is consistent! For example, when I updated SearchField, I updated it to use the |
| } | ||
|
|
||
| const startIconElement = React.cloneElement(startIcon, { | ||
| ...startIcon.props, |
There was a problem hiding this comment.
suggestion: (no changes necessary) What do you think about providing a default size and color if the color and size props aren't defined on the startIcon? That way, teams could pass in the PhosphorIcon without having to worry about what color and size to provide, and they can if they need to override it!
| /** | ||
| * Start icon styles | ||
| */ | ||
| iconDisabled: { |
There was a problem hiding this comment.
nit: I think this isn't used!
| }, | ||
| args: { | ||
| children: items, | ||
| startIcon: ( |
There was a problem hiding this comment.
Let's consolidate on the magnifying icon styling! In SearchField, I updated the following to match the design more closely:
size=smallcolor=semanticColor.icon.primary- use the bold icon instead
@phosphor-icons/core/bold/magnifying-glass-bold.svg
Let me know though if we prefer to stick with the existing styles, I can update SearchField to change it back to how it was! (I was assuming Figma was the source of truth for styling, but that may not be the case!)
There was a problem hiding this comment.
Sounds great! I hope GH includes the 💯 emoji in their msg reactions at some point.
To clarify after our last convo, size would be small right? (medium was for the dismiss icon).
There was a problem hiding this comment.
The magnifying glass PhosphorIcon size will be small!
The clear/dismiss IconButton size is also small and the x icon is the regular icon weight!
They're both "small", but the PhosphorIcon renders a 16x16 icon and IconButton renders a 24x24 icon!
| <Combobox | ||
| {...args} | ||
| startIcon={ | ||
| <PhosphorIcon icon={magnifyingGlassIcon} size="small" /> |
There was a problem hiding this comment.
suggestion: We can remove size="small" here so we can confirm that the default size is being applied!
There was a problem hiding this comment.
Good catch! I'll fix it before landing.
Summary:
Adds a new
startIconprop (optional) to the Combobox component. This propwill allow to optinally add an icon to the left of the input field.
This use case will be helpful for the AI tools case where we want to use a
search icon.
Issue: https://khanacademy.atlassian.net/browse/WB-1779
Test plan:
Verify that the
Start icondocs make sense and that the example works asexpected.
/?path=/docs/packages-dropdown-combobox--docs#start%20icon