Toolbar: Add unstyled variant #55139
Merged
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
brookewp
added
[Type] Bug
brookewp
requested review from
andrewhayward,
chad1008,
ciampo,
jasmussen and
tyxla
brookewp
commented
Oct 6, 2023
|
Size Change: +58 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
brookewp
commented
Oct 6, 2023
| * Otherwise, leave it as `undefined` for default styles. | ||
| */ | ||
|
|
||
| export const WithoutStyles = Template.bind( {} ); |
Contributor
Author
There was a problem hiding this comment.
I mentioned consistency with Popover...
So if we decide to keep the name 'unstyled', I'll update the story name to match.
Contributor
There was a problem hiding this comment.
I'll update the story name to match.
Yeah, "Unstyled" sounds like a better name for this story.
Also, I think we could get away with re-using the default story's arguments — ie.
WithoutStyles.args = {
...Default.args,
variant: 'unstyled',
};And even more in general, I wonder if we could avoid adding a new story altogether, since folks could just use the variant controls on existing stories?
Contributor
Author
There was a problem hiding this comment.
We could leave the story out. I added it because it's a visual change, so I thought seeing an explicit demo could be helpful. Also, Popover has the same story so I can add another point to my consistency list 😄
But I'm happy to remove it if you don't think it adds enough value.
Contributor
There was a problem hiding this comment.
Let's keep it for now, but maybe rewrite the args to reuse Default.args like suggested above?
|
Flaky tests detected in 74d2c30. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6487678810
|
tyxla
reviewed
Oct 9, 2023
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
I'd let the API design review to others who already have more context, but in the meantime, I've left some minor drive-by improvement suggestions.
packages/edit-post/src/components/header/header-toolbar/index.js
Outdated
Show resolved
Hide resolved
Comment on lines
31
to
44
| const CONTEXT_SYSTEM_VALUE = useMemo( () => { | ||
| if ( variant !== undefined ) { | ||
| return {}; | ||
| } | ||
|
|
||
| return { | ||
| DropdownMenu: { | ||
| variant: 'toolbar', | ||
| }, | ||
| Dropdown: { | ||
| variant: 'toolbar', | ||
| }, | ||
| }; | ||
| }, [ variant ] ); |
Member
There was a problem hiding this comment.
CONTEXT_SYSTEM_VALUE will be recalculated on every value, regardless what it is. At the same time, we only care about its declaration if it changes from undefined to any other value, or the other way around.
So, to micro-optimize, I'd recommend declaring that as a value above the useMemo and using that instead of variant:
Suggested change
| const CONTEXT_SYSTEM_VALUE = useMemo( () => { | |
| if ( variant !== undefined ) { | |
| return {}; | |
| } | |
| return { | |
| DropdownMenu: { | |
| variant: 'toolbar', | |
| }, | |
| Dropdown: { | |
| variant: 'toolbar', | |
| }, | |
| }; | |
| }, [ variant ] ); | |
| const isVariantDefined = variant !== undefined; | |
| const CONTEXT_SYSTEM_VALUE = useMemo( () => { | |
| if ( isVariantDefined ) { | |
| return {}; | |
| } | |
| return { | |
| DropdownMenu: { | |
| variant: 'toolbar', | |
| }, | |
| Dropdown: { | |
| variant: 'toolbar', | |
| }, | |
| }; | |
| }, [ isVariantDefined ] ); |
Contributor
Author
There was a problem hiding this comment.
Thanks for sharing! I'm unfamiliar with useMemo and implemented it due to a console.warn. So, I'm unsure if I fully understand the difference. 🤔 If you have time to share more or if you know of any examples I could look into to understand further, I'd appreciate it!
Contributor
There was a problem hiding this comment.
The code inside useMemo gets executed only when any of its dependencies change — it is therefore useful when we want to only re-compute something when absolutely necessary, and otherwise use the result from past computations. This is particularly useful for a bunch of reasons:
- it's great when we need to compute something particularly complex / slow to calculate (not this case)
- since React components re-render any time one of their props change, by memoizing a value we can try to reduce that number of rerenders. In fact, without
useMemowe would be technically creating a new, different object every time
The react docs are a good start if you're interested in learning more
Member
There was a problem hiding this comment.
What @ciampo said 👍
In terms of how often it recomputes, useMemo() is very similar to useEffect or useCallback - it will be recomputed only if one of the dependencies change, so we always strive to keep the dependencies as minimal and as static as possible.
Let's consider an example. If variant is a dependency, then the useMemo() will be recompute every time variant changes, for example, if variant was A and gets changed to B, or if it was B and was changed to undefined. However, if we declare const isVariantDefined = variant !== undefined; and then use isVariantDefined as a dependency, useMemo will recompute only if isVariantDefined changes from undefined to anything else, or from anything else to undefined. That means, if variant changes from A to B, isVariantDefined won't change, therefore useMemo() won't recompute.
That being said, it's a micro-optimization, and likely won't have a measurable impact, unless we change the variant prop often for some reason.
| }: WordPressComponentProps< ToolbarProps, 'div', false >, | ||
| ref: ForwardedRef< any > | ||
| ) { | ||
| const CONTEXT_SYSTEM_VALUE = useMemo( () => { |
Member
There was a problem hiding this comment.
I'd encourage changing CONTEXT_SYSTEM_VALUE to contextSystemValue since this is no longer a module constant.
Contributor
Author
There was a problem hiding this comment.
I'm also unfamiliar with the term 'module constants' so thanks for suggesting! My assumption on this change is now there is more than one possible return value vs. before. Is that correct, or is it based on something else?
Contributor
There was a problem hiding this comment.
This simply a naming convention: constant values variables are usually UPPERCASE_LIKE_THIS. That was the case for CONTEXT_SYSTEM_VALUE before this PR.
But since it's been moved inside the react component's function, it's no longer a constant. Marin is suggesting that we change the casing (if that's even a word) to a more traditional camelCase
Member
There was a problem hiding this comment.
Thanks for elaborating, @ciampo, that's exactly what I meant.
Contributor
|
Thanks for keeping on this! From just discerning the before and after, this looks right, good job! I'll defer to others for a code review, but let me know if you need me to dive in and test more deeply. |
ciampo
reviewed
Oct 10, 2023
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
This is testing well for me too. Left a couple of suggestions!
| * Otherwise, leave it as `undefined` for default styles. | ||
| */ | ||
|
|
||
| export const WithoutStyles = Template.bind( {} ); |
Contributor
There was a problem hiding this comment.
I'll update the story name to match.
Yeah, "Unstyled" sounds like a better name for this story.
Also, I think we could get away with re-using the default story's arguments — ie.
WithoutStyles.args = {
...Default.args,
variant: 'unstyled',
};And even more in general, I wonder if we could avoid adding a new story altogether, since folks could just use the variant controls on existing stories?
# Conflicts: # packages/components/CHANGELOG.md
ciampo
added
the
Needs Dev Note
brookewp
added
has dev note
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?
Closes #54868
This covers a missing requirement where the
Toolbarcomponent shouldn't always use thetoolbarvariant ofPopoverforDropdownMenuwhen in the block editor. @jasmussen clarified the requirements:Why?
This was discovered as a bug in #54840 and a fix was added to remove the variant.
As a longer-term solution, @ciampo suggested we add a variant to
Toolbarso we can remove existing overrides (which also include CSS overrides) and avoid adding more in the future.How?
As suggested, an 'unstyled'
varianthas been added toToolbar. If this is set, theDropdownMenu'sPopoverwill be set to the default (undefined) value.This PR also removes the overrides added, the one mentioned above, and CSS overrides. See these changes in the testing steps below.
Testing Instructions
The main thing to look for is that the
Toolbarcomponents appear as they did before, with no regressions. It should only appear differently in the blocks where an 'unstyled'varianthas been set.BlockContextualToolbar
The only change here should be on smaller screens when the toolbar becomes full width and to a lighter border. Its dropdown should now match:
Otherwise, the block toolbar and its dropdowns should appear as it did before:
HeaderToolbar
The override added in #54840 is removed from
ToolSelectorbut is resolved by adding the variant toHeaderToolbar, which allows us to remove the CSS override added before. This should continue to appear the same as it does in trunk currently.✍️ Dev note
A new prop,
variant, has been added to the Toolbar component to add more flexibility in styling. This allows consumers to adopt an'unstyled'option which removes default border styles, so fewer manual overrides will be needed. If the prop isundefined, the Toolbar will retain its default border styles.