Add a popover variant prop and refactor popovers to use it, deprecate isAlternate
#45137
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
talldan
added
[Type] Code Quality
talldan
requested review from
ellatrix,
fluiddot and
ajitbohra
as code owners
|
|
|
Size Change: +66 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
talldan
commented
Oct 20, 2022
67 tasks
mirka
reviewed
Oct 20, 2022
Comment on lines
144
to
145
| * - 'alternate': A style that has no elevation, but a high contrast with | ||
| * other elements. |
There was a problem hiding this comment.
Would this be a good opportunity to give the "alternate" variant a more meaningful name? Even something like highContrast as you put it in this description feels better to me. Would that be an appropriate encapsulation of the design intent? (cc @jasmussen)
There was a problem hiding this comment.
That's a good shout, I wondered that too.
There was a problem hiding this comment.
Thanks for the ping. I don't have a strong opinion on the name, though I don't think we should call it high-contrast. In fact ideally the variant name can become entirely hidden and automatic. The thing is: when a popover opens from the block toolbar, the popover should be the same visual style as the block toolbar (with the dark border). If it doesn't open from a block toolbar, it should be the default shadowed light gray material. Could this be made automatic so as a dev, one doesn't have to choose this variant?
There was a problem hiding this comment.
There was a problem hiding this comment.
'toolbar' sounds like a good idea 👍
There was a problem hiding this comment.
I've updated the PR to use that naming.
There was a problem hiding this comment.
👋 from the future. We now have top toolbar which has different border settings. The floating toolbar popover has a higher contrast black border and the top toolbar has the default shadowed gray border. They are both opened from the toolbar, so the name toolbar no longer communicates the difference. I think changing it to something that indicates the style like highContrast, etc makes more sense now.
I've looked into this a little bit and it seems there's a lot to unravel here, so I'm sure there's some context and considerations I'm missing. Given the current state of the toolbars, what do y'all think?
There was a problem hiding this comment.
Naming things is hard — the toolbar naming was in place to suggest that the popover would look exactly like the toolbar — but then the toolbar also got a style variant.
highContrast sounds like a more descriptive name, although that means that, should the styles required by the toolbar change to something that doesn't look very "high contrast", we will probably have to introduce a new variant, and keep highContrast around.
If were to implement those changes, we would have to deprecate the toolbar variant name, keep it around for backwards compat, and refactot the toolbar, dropdown and dropdownmenu components to use the new highContrast variant instead.
ciampo
reviewed
Oct 20, 2022
Comment on lines
240
to
242
| - `alternate`: A style that has no elevation, but a high contrast with other elements. | ||
|
|
||
|
|
There was a problem hiding this comment.
Suggested change
| - `alternate`: A style that has no elevation, but a high contrast with other elements. | |
| - `alternate`: A style that has no elevation, but a high contrast with other elements. | |
There was a problem hiding this comment.
🤔 Made that change, but it didn't seem to fix the issue.
There was a problem hiding this comment.
Maybe I formatted the suggestion incorrectly — I was just trying to remove the double emtpy line 😅
There was a problem hiding this comment.
Ah, I see. When rendered in markdown it's becoming one list with the - Required: No entry as the last item. I thought that's what you were referring to. No matter how many blank lines are there it still happens.
On Stack Overflow it was mentioned that you can use an HTML comment to separate lists, so I've done that. It's not a great solution. Visually it still looks a bit like one list.
| @@ -476,7 +488,7 @@ const UnforwardedPopover = ( | |||
| placement={ computedPlacement } | |||
| className={ classnames( 'components-popover', className, { | |||
| 'is-expanded': isExpanded, | |||
| 'is-alternate': isAlternate, | |||
| [ `is-${ computedVariant }` ]: !! computedVariant, | |||
There was a problem hiding this comment.
if we change name of the alternate variant, should we still look into keeping the is-alternate classname for that specific variant for legacy reasons?
There was a problem hiding this comment.
That's a good point. Technically we can change it according to the back compat doc:
https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/backward-compatibility.md#class-names-and-dom-updates
But it may not be worth it as it could cause some breakage. It would be good to keep it in sync with the prop value to avoid unnecessary branching code. So that would be an argument for keeping 'alternate'.
There was a problem hiding this comment.
I'd be in favour of changing the variant name to something else (i.e. highContrast), but then mapping that variant to the is-alternate classname for back compat.
There was a problem hiding this comment.
It's changed to 'toolbar' now as per the other conversation (#45137 (comment)), it still outputs the is-alternate classname.
|
I've pushed a commit to address some feedback (and resolved those conversations). |
talldan
force-pushed
the
add/frameless-popover-variation
branch
3 times, most recently
from
f9e110b
to
6a184ff
Compare
|
I've pushed some further adjustments based on the feedback, and also added a couple of stories for the new variants. |
mirka
reviewed
Oct 26, 2022
Call it unstyled instead of frameless Also reset border radius (ensure no clipping of content) Make the list view drop indicator popover unstyled Make the block popover an unstyled popover Use unstyled variant for inbetween popover Polish types and update docs Switch from deprecated `isVariant` popover prop to `variant: alternate` Variant is a stable prop Update changelog Update docs Switch from alternate to toolbar Add stories Remove strict equality check Remove deprecation message in docs Co-authored-by: Lena Morita <lena@jaguchi.com> Remove specification of options for variant prop in storybook Co-authored-by: Lena Morita <lena@jaguchi.com> Simplify storybook code
talldan
force-pushed
the
add/frameless-popover-variation
branch
from
cff7ca2
to
0687a9f
Compare
|
All feedback should be addressed now. Hopefully this one's ready to go. |
Merged
6 tasks
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? How?
See #42770
In a few places in the codebase there are popovers with styles overridden to remove border, background, box shadow and outline:
This replaces those custom styles by adding a new
variant="unstyled"prop, which matches the approach in theButtoncomponent.The popover also already had an
isAlternateprop that styles the popover differently. In this PR I'm also deprecating that and replacing it withvariant="toolbar"(which still outputs theis-alternateclassname for backwards compatibility).Why?
isAlternateisn't scalable.Alternatives considered
Introducing a
Floatingcomponent that's a base component forPopoverand has no styles. There are a couple of reasons I don't think this will work well. Firstly it doesn't deal withisAlternate. Secondly, popovers (and possible this new floating component) render an outer and inner element, andPopoverwould end up needing to style these.Testing Instructions
trunkScreenshots or screencast
Visualizers
Inbetween inserter
Toolbar dropdowns