(Custom)SelectControl: Refresh and refactor chevron #42962
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mirka
added
[Type] Enhancement
mirka
commented
Aug 4, 2022
|
Size Change: +3.44 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
|
Nice! This looks pretty good to me. Here's the reference I'm looking at for comparison:
As I mentioned on another thread, I'm refreshing my memory and coming back to the conversation about size defaults — this defaults to 40px correct? |
No, the default still remains 30px. I'm inching closer to getting 40px variants enabled in the sidebar though. I'm planning on flipping the switch beginning with the Typography panel in the Global Styles sidebar, since that's pretty contained. |
ciampo
reviewed
Aug 8, 2022
There was a problem hiding this comment.
Thank you for working on this!
Currently, when I try to click on the chevron icon, the dropdowns (both in SelectControl and CustomSelectControl) don't react to the pointer events (clicking on the rest of the input works)
Also, the layout of SelectControl looks a bit broken when switching to "Wordpress (common/forms)" in the global CSS switcher
packages/components/src/select-control/styles/select-control-styles.ts
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| const backCompatMinWidth = ( props: BackCompatMinWidthProps ) => | ||
| ! props.__nextUnconstrainedWidth |
There was a problem hiding this comment.
It looks like InputBaseWithBackCompatMinWidth never receives the __nextUnconstrainedWidth prop in packages/components/src/custom-select-control/index.js
There was a problem hiding this comment.
Lol, I like how this still accidentally works even without the correct prop 😂 (Also how do people code without end-to-end typescript checking...)
# Conflicts: # packages/components/CHANGELOG.md # packages/components/src/custom-select-control/index.js
Great one, thank you. Fixed in f5f5639.
Congrats, this observation caught two pre-existing forms.css problems:
Thanks for the great review @ciampo! All feedback is now addressed. |
ciampo
reviewed
Aug 11, 2022
packages/components/src/select-control/styles/select-control-styles.ts
Outdated
Show resolved
Hide resolved
packages/components/src/select-control/styles/select-control-styles.ts
Outdated
Show resolved
Hide resolved
|
@ciampo What would we do without you, Marco. Thanks for the 👀 ! |
ciampo
approved these changes
Aug 11, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to #42460
What?
Refactors the
chevronDownicon in SelectControl/CustomSelectControl so the implementation is clean and the visual is consistent.Visual changes
@jasmussen Are we happy with the size of the icon? And the spacing before and after it? Let me know if you have tweaks.
Why?
We now have a clean way to implement the chevron (
suffixprop & suffix wrapper component). It removes hacks and maintains visual consistency across components.How?
Refactor the implementation to use the
suffixprop & suffix wrapper component.Testing Instructions
npm run storybook:dev✅ Works in RTL
✅ Width behavior is maintained for CustomSelectControl. (130px wide by default, and grows when the label is long)
Screenshots or screencast
Here's a video of all the size variants (24, 30, 36, 40):
CleanShot.2022-08-04.at.14.51.44.mp4