Use internal context system to apply toolbar variant to toolbar dropdowns #51154
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
|
Size Change: -41 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
@ciampo I pushed a commit that implements passing the components context to the Fill. How it works:
Here's the alignment matrix control with the correct dark border color: the 
Open problems:
|
ciampo
force-pushed
the
feat/dropdownmenu-toolbar-variant-via-context
branch
from
0167225
to
76e633a
Compare
ciampo
added
[Package] Components
ciampo
commented
Jun 6, 2023
ciampo
commented
Jun 6, 2023
| position: 'bottom right', | ||
| variant: 'toolbar', | ||
| } } | ||
| popoverProps={ POPOVER_PROPS } |
There was a problem hiding this comment.
Moved to a separate variable to avoid creating a new object every render
ciampo
commented
Jun 6, 2023
| }, | ||
| popoverProps | ||
| ); | ||
|
|
||
| return ( | ||
| <Dropdown | ||
| className={ classnames( 'components-dropdown-menu', className ) } | ||
| className={ className } |
There was a problem hiding this comment.
The context system automatically generates a components-* classname based on which namespace is passed in the useContextSystem hook — therefore making the components-dropdown-menu hardcoded classname unnecessary, since duplicated.
The same change was applied to the Dropdown component
ciampo
commented
Jun 6, 2023
| @@ -128,19 +128,6 @@ Default.args = { | |||
| ), | |||
| }; | |||
|
|
|||
| export const Toolbar: ComponentStory< typeof Popover > = Template.bind( {} ); | |||
There was a problem hiding this comment.
This Storybook example is being removed since we don't want to incentivize the usage of the variant: "toolbar" prop on the Popover component. The prop is still around and can be tweaked in all Storybook examples — there simply won't be an example dedicated to the toolbar variant.
|
While waiting for progress on #51264, I think we can start giving an initial round of review to this PR |
ciampo
requested review from
ellatrix,
fluiddot,
dcalhoun,
ajitbohra and
tellthemachines
as code owners
ciampo
requested review from
talldan,
jasmussen,
tyxla,
chad1008,
andrewserong,
ntsekouras and
aaronrobertshaw
There was a problem hiding this comment.
Nice work so far on this one @ciampo 👍
I didn't spot any differences in popover placement between this PR and trunk although I did encounter some weird placement of the RichText toolbar. That could be replicated on trunk however.
Screen.Recording.2023-06-07.at.5.25.58.pm.mp4
I've left an inline question on whether the ComponentsContext should be exported via the private APIs but other than that, I think this is pretty close.
|
Nice. Trunk shows the wrong style for Query loop: 
This branch shows the correct popover style for the Query loop: 
|
ciampo
force-pushed
the
feat/dropdownmenu-toolbar-variant-via-context
branch
from
155145e
to
af98989
Compare
|
With #51264 merged, I removed the extra slotfill changes from this PR and rebased on top of This PR is not ready for a final review. |
aaronrobertshaw
approved these changes
Jun 8, 2023
There was a problem hiding this comment.
Thanks for continuing to wrangle this one 👍
The code changes look good and the various dropdowns display nicely for me.
✅ Alignment & alignment matrix controls
✅ Block settings dropdown
✅ Block switcher
✅ Duotone control
✅ Image Editor
✅ Media replacement
✅ Block editor preview options
✅ RichText format toolbar
✅ Flex layout controls
✅ Navigation leaf more menu
✅ Site editor nav screen leaf more menu
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…owns (WordPress#51154) * DropdownMenu: read variant from context * Toolbar: do not set `variant` prop on popover (rely on context system instead) * Use named export for better storybook doc generation * Provide context value in more toolbar components * Hook Dropdown into context system, update DropdownMenu and CircularOptionPicker * Remove toolbar variant popover prop where the context system can be used instead * Add comment where the context system currently doesn't work * Use placement prop instead of legacy "position" prop * Add some temporary TODO comments * Set toolbar variant for Dropdown in the context system * Remove explicit variant prop from block alignment matrix control * Toolbar: Only set context in the top-level component * Remove `toolbar` variant from instances using `Dropdown` * Simplify Dropdown implementation * Tidy up types * Remove toolbar variant storybook example for Popover * CHANGELOG * Complete renaming internal type * Remove duplicate hardcoded classnames(already added by context connexct)
This was referenced Sep 27, 2023
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.
What?
Needs #51264 to be merged first.
This PR refactors usages of
DropdownMenuandDropdownacross the repository, removing the need for passing explicitly thevariant: "toolbar"prop — the components are expected to pick the correct visual variant automatically when rendered inside aToolbar.Why?
For a few reasons:
popoverPropsless explicitly allows for cleaner code. I also think that the requirement of "dropdown menus needing to render differently when in a toolbar" should not be achieved by expecting that the consumers of the component explicitly set a prop.Most importantly, the new version of
DropdownMenuthat is in the works already relies on the components context system for switching visual style. Updating the "legacy" version of the component should ensure a smoother transition to the new component.How?
Toolbarcomponent now uses the components context system to set avariantforDropdownMenuandDropdowncomponents.DropdownMenuandDropdowncomponents now connect to the components context system, look for thevariantproperty passed via context, and set it appropriately on the underlyingPopovercomponentvariant: "toolbar"is explicitly set in the block editor, since all dropdowns should now render with the correct style variation automatically.variantprops, I've also replaced instances of thepositionprop with theplacementprop, since thepositionprop is gradually being phased outTesting Instructions
In the block editor and in the site editor select a variety of blocks, and interact with the block toolbar. Make sure that every single dropdown rendered in the toolbar has the same visual styles as on
trunkScreenshots or screencast