Dropdown: Add wrapper for custom padding #42595
Merged
+160
−69
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
commented
Jul 21, 2022
Comment on lines
-4
to
-10
| import { | ||
| more, | ||
| arrowLeft, | ||
| arrowRight, | ||
| arrowUp, | ||
| arrowDown, | ||
| } from '@wordpress/icons'; |
There was a problem hiding this comment.
I had to do some basic cleanup of the story first. It may be easier to review the commits separately.
|
Size Change: +146 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
1 task
|
Here's what I'm seeing: padding.mp4
It's working well for me. |
ciampo
approved these changes
Jul 21, 2022
There was a problem hiding this comment.
LGTM 🚀
PS: the new story revealed some weird behaviors:
- the layout on small viewports looks a bit weird when
expandOnMobileistrue - the
headerTitleis only shown on mobile viewports whenexpandOnMobileistrue(it's documented in the README, but it's still a bit weird)
|
✅ Styling bug with |
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 #41937, #41900
What?
Adds a
<DropdownContentWrapper>subcomponent for managing different padding values in theDropdowncontent.Why/How?
We have encountered many ad hoc overrides of the Dropdown content padding, which involve reaching into internal CSS classes (
.components-popover__content).Size scale
A survey of all the padding overrides in the codebase showed that almost all these instances wanted either a padding value of 16px or 0 (default is 8px). The only two exceptions were rather arbitrary values, not adhering to the 4px grid system.
This suggests that we can abstract this to some padding presets. There is an existing padding size scale used in
Card, but I'm hesitant to adopt the same scale because the context is different. I'm leaning toward a scale that is specific to Dropdown usage, but this is up for discussion.My tentative proposal is
small=8px(default),medium=16px, andnone=0.Prop or subcomponent
The other main decision we need to make is whether to add a root-level
contentPaddingSizeprop, or to add a wrapper subcomponent.Root-level props are seemingly simpler, but always come with the slippery slope risk of cluttering up the root component with increasingly niche props. Subcomponents are safer in that regard, as it will be a good vessel to add more content-related props in the future.
That said, the main reason I'm leaning toward a subcomponent in this case because we are seeing a high number of anti-patterns where the consumer is setting dimension styles (e.g.
min-width) directly on.components-popover__content. This is easily avoidable by using their own wrapper div on the dropdown content. By providing an "official" wrapper component, maybe we can further encourage consumers to set styles on wrappers instead of reaching into internal CSS classes.Testing Instructions
1.
npm run storybook:devand see the stories forDropdown.