DropdownMenu v2: tweak styles, add toolbar-specific styles #51097
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
ciampo
added
[Feature] UI Components
ciampo
commented
May 30, 2023
ciampo
commented
May 30, 2023
| @@ -4,6 +4,7 @@ export { | |||
| } from './context-system-provider'; | |||
| export { | |||
| contextConnect, | |||
| contextConnectWithoutRef, | |||
There was a problem hiding this comment.
This function was added in #43611 but was never added to the list of exports
ciampo
force-pushed
the
feat/dropdownmenu-v2-toolbar-styles
branch
from
8e45f74 to
6e46fca
Compare
ciampo
requested review from
mirka,
SaxonF,
jasmussen,
tyxla,
chad1008,
andrewserong and
jameskoster
This was referenced May 30, 2023
ciampo
commented
May 30, 2023
| ref={ ref } | ||
| { ...props } | ||
| /> | ||
| <ContextSystemProvider value={ CONTEXT_SYSTEM_VALUE }> |
There was a problem hiding this comment.
This won't have any effect on dropdowns inside Toolbar just yet, since it's still using the legacy DropdownMenu component. But it's setting the ground.
|
Flaky tests detected in 0887983. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5132767678
|
tyxla
approved these changes
May 31, 2023
| @@ -23,6 +24,14 @@ const ITEM_PREFIX_WIDTH = space( 7 ); | |||
| const ITEM_PADDING_INLINE_START = space( 2 ); | |||
| const ITEM_PADDING_INLINE_END = space( 2.5 ); | |||
|
|
|||
| // TODO: should bring this into the config, and make themeable | |||
|
This is cool, thank you for working on this. A couple of screenshots: 
Short answer: this component is going to be such an improvement to our menus, that I would love to see a version of this land in short order. In that light, I wouldn't call any of the following feedback to be blocking for the PR to land, but rather a wishlist or laundry-list of todos, weighted according to priority. First off, things I'd love to address in this PR, or shortly after, if at all possible: First off, apply the same box shadow we have for existing popovers at the moment. That's Secondly, this spacing is both small and uneven. I have a sense this might be tricky to improve, but if possible we should have some larger spacing here to feel balanced with the dark outlines. I think the original design calls for 8 or 12px, but we can try 6px as well. Right now it's between 1.5px and 2px spacing, likely due to subpixel positioning issues. As noted, probably a tricky one, but anything we can do here would be good. In the category of smaller things, again, things we can live without but nice if possible nevertheless. This icon looks to be 18x18. With almost no exceptions, our icons should always be 24x24: 
Which is curious because the check looks right: 
This helper text at 10px is fairly small and #777777. I think we use 12px helper text that should be #757575. Let me know if that's helpful! |
ciampo
force-pushed
the
feat/dropdownmenu-v2-toolbar-styles
branch
from
aa9b42f to
8e711f9
Compare
ciampo
changed the title
|
Size Change: +450 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
Rebased to solve conflicts and include changes from #51046.
Since my understanding is that we want this to look like the current dropdown/popover, I've also added a light gray border to the box shadow (as per the current popover styles). Let me know if that's ok.
I assumed you're referring to the space between submenu popovers.I went ahead and I increased the gap by Let me know if that's what you intended, and if the change looks good. Also, should that gap should be different based on toolbar styles being applied, or should it be the same for all instanced of
That 18x18 icon is just part of the Storybook example, and it's not really related to the I believe it is a decision that I took after implementing the current design specs. The 24x24 version of the WordPress icon felt a bit disproportionate, especially given the space between the icon and the text to its right. I've updated the Storybook example to use a 24x24 icon, let me know what you think.
The check icon is part of the
The helper text is also just part of the Storybook example (and not part of the For ease of review, this is how the Storybook examples look like after the latest set of changes:
|
ciampo
changed the title
|
Let's try 4px now, and yes if these new ones can look identical to the existing popovers (seems like that's actually the case), then we can definitely retire the old ones sooner, that'd be nice. So much we can clean up |
|
Cool, thank you! I'm going to merge this PR, but feel free to open a new issue with more tweaks as you see fit |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…#51097) * Update border radius to 2px * Add toolbar variant styles to top content wrapper * Add toolbar variant styles to sub content wrapper and separators via internal react context * Connect `DropdownMenu` to the components context system * Add temporary Storybook example * Memoize internal context value to avoid extra re-renders * CHANGELOG * Set the toolbar dropdown menu variant in the toolbar component via the context system * Tweak box shadow to match the existing popover * Update icon size and helper text styles in Storybook example * Tweak sub menu offset * add TODO comment
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.
Part of #50459
What?
Refine
DropdownMenuv2 styles:2px;Popovercomponent;variantthat can be set via only by other components in@wordpress/components, which causes:Note: this PR only focuses on tweaking styles on the
DropdownMenucomponent. It does NOT make any changes to theToolbarandToolbarDropdowncomponents (and therefore it doesn't impact current dropdowns in the editor). Those changes will be part of a separate PR, and will need some thorough testing around integrating the newDropdownMenucomponent in the editor.Why?
Adapting to design specs.
How?
The answer is mostly React Context:
variantproperty. This property can only be set using the packages' context system, meaning only other components from@wordpress/componentswill be able to trigger this variant. The plan is for this property to be set by theToolbarcomponentsDropdownMenucomponent, we use another instance of React Content to propagate the receivedvariantdown to all subcomponents. This is to ensure that also the sub-content wrapper and the separator can render accordingly to the chosen variant.I will add more details about the implementation through inline comments.
Testing Instructions
For now, the only way to test the new styles is via Storybook:
2pxfor both the content wrappers and each itemToolbar VariantStorybook example, and notice how the component renders with darker borders (including nested menus and separators).Screenshots or screencast
drodown-menu-toolbar.mp4