Border Controls: Passthrough popover props instead of class names #40836
Conversation
This file contains hidden or 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
aaronrobertshaw
added
[Type] Enhancement
aaronrobertshaw
commented
May 5, 2022
packages/components/src/border-control/border-control-dropdown/component.tsx
Outdated
Show resolved
Hide resolved
|
Size Change: +3.94 kB (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
May 5, 2022
youknowriad
reviewed
May 5, 2022
Contributor
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
Thanks for this change, this will definitely help me in my refactoring Popover PR :)
ciampo
reviewed
May 5, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
The changes look correct to me.
While I understand the reasoning behind these changes, and I acknowledge that the Dropdown component also exposes a popoverProps prop, I'd like to make sure that this solution is a good pattern to adopt moving forward. especially in terms of components composition and future maintainability.
6 tasks
Contributor
Author
|
Closing this PR due to a new approach that will be adopted as part of #40740. |
youknowriad
deleted the
update/border-controls-to-passthrough-popover-props
branch
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.
Related:
What?
Updates the
BorderControlandBorderBoxControlcomponents to pass throughpopoverPropsvia their internal dropdowns instead of CSS classnames.Why?
This change is to provide greater flexibility and unblock new work refactoring the
Popovercomponent. Given the border control components are still experimental this change in their API should be ok.How?
popoverPropsas a means for plumbing through class names.BorderBoxControl's newpopoversPropsprop (any ideas on better naming here would be appreciated).Note: The class names are still relied upon to provide the correct positioning of the border color/style dropdowns in the block editor. This will change in a followup as #40740 proceeds.
Testing Instructions
npm run test-unit packages/components/src/border-control/test/index.jsnpm run test-unit packages/components/src/border-box-control/test/index.jsnpm run build:package-typesScreenshots or screencast
Screen.Recording.2022-05-05.at.12.54.46.pm.mp4