Open
Conversation
|
|
0338cae to
6f4b7a6
Compare
7327210 to
276c808
Compare
9588263 to
5591154
Compare
# Conflicts: # packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx # packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx # packages/components/src/toggle-group-control/types.ts # Conflicts: # packages/components/src/toggle-group-control/toggle-group-control-option-base/component.tsx # packages/components/src/toggle-group-control/toggle-group-control-option-base/styles.ts
# Conflicts: # packages/components/src/toggle-group-control/toggle-group-control-option-base/styles.ts
276c808 to
1d41c30
Compare
d9121c2 to
89e4acf
Compare
|
Size Change: +217 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
mirka
commented
Nov 7, 2022
| isAdaptiveWidth={ isAdaptiveWidth } | ||
| /> | ||
| { children } | ||
| <HStack spacing={ 1 }>{ children }</HStack> |
Member
Author
There was a problem hiding this comment.
Spacing was a bit tight to begin with in the isDeselectable ToggleGroupControl, but it becomes very noticeable when multiple buttons are pressed. Added some breathing room.
| ToggleGroupControlProps, | ||
| ToggleGroupControlOptionBaseProps, | ||
| } from '../types'; | ||
| import { BACKDROP_BG_COLOR } from '../toggle-group-control/styles'; |
Member
Author
There was a problem hiding this comment.
The changes in this style file is to allow a pressed button's black background to remain "static", unlike the animated roving background in the single-select variant.
Contributor
|
Nice, looks good:
Not sure if it needs to be this PR, but the design for sequential toggles has the toggled states at 32x32 with 8px gap between them: That should leave enough room for multiple toggled. Will this component replace items in the block toolbar (as in the Bold/Italic example), or does it have a different purpose? |
Member
Author
|
Thanks for reviewing! I increased the button gap to 8px. The button size will be tweaked separately (#45532).
No, this is not for the block toolbar. Just for sidebar controls like Decoration, Line Height, that kind of thing. |
Contributor
|
Sounds good. But conceptually is it the same mechanism, or different? UI wise they should be the same, but I'm curiuos of the inner workings, if you don't mind. In any case, this seems good to me, since we're following up separately on the 32x32 footprint. Just needs a code review 👍 👍 |
Member
Author
I love this question! Semantically, they are similar in that they both contain
When we zoom out from the button and consider the whole, they become conceptually different. In the Toolbar, keyboard navigation is completely managed by the Toolbar itself, and the buttons are not semantically grouped. In a sense, they are each just independent buttons in a toolbar that happen to be visually separated by vertical dividers. In ToggleGroupControl, the buttons are semantically grouped together ( |
Contributor
|
Thank you for indulging my question! 🙏 |
ciampo
reviewed
Nov 15, 2022
Contributor
ciampo
left a comment
There was a problem hiding this comment.
Thank you for working on this! We're getting closer and closer :)
| const singleSelectButtonProps = { | ||
| 'aria-pressed': isPressed, | ||
| onClick: () => | ||
| otherContextProps.setState( isPressed ? undefined : value ), |
Contributor
There was a problem hiding this comment.
Previously, undefined was passed as the new state only when isDeselectable === true — is it ok to remove that check?
Comment on lines
+159
to
+162
| export { | ||
| ToggleMultipleGroupControl as __experimentalToggleMultipleGroupControl, | ||
| ToggleMultipleGroupControlOptionIcon as __experimentalToggleMultipleGroupControlOptionIcon, | ||
| } from './toggle-multiple-group-control'; |
Contributor
There was a problem hiding this comment.
With @adamziel we recently discussed using the IS_GUTENBERG_PLUGIN env variable, so that we can prevent these experimental components from making their way in a Core release, effectively keeping them truly experimental.
Adam, could you help us understand a bit better what using IS_GUTENBERG_PLUGIN would look like in this scenario? Thank you 🙏
| * | ||
| * @default false | ||
| */ | ||
| isMultiple?: boolean; |
Contributor
There was a problem hiding this comment.
Do we need to update ToggleGroupControlOptionBase and ToggleGroupControlOptionIcon's READMEs with docs about this new prop?
| forwardedRef: ForwardedRef< any > | ||
| ) { | ||
| const { | ||
| onChange, // omit |
Contributor
There was a problem hiding this comment.
What's this inline comment about ?
| /** | ||
| * Listen to click events to manage the `isPressed` state. | ||
| */ | ||
| onClick: React.MouseEventHandler< HTMLButtonElement >; |
Contributor
There was a problem hiding this comment.
Thought: if we allow this component to be polymorphic, then we're not guaranteed about the HTMLButtonElement part on the onClick event?
| { | ||
| isPressed, | ||
| ...otherProps | ||
| }: WordPressComponentProps< |
Contributor
There was a problem hiding this comment.
Should we explicitly Omit the aria-pressed attribute from WordPressComponentProps here?
|
|
||
| ### `value`: `string | number` | ||
|
|
||
| The unique key of the option. |
Contributor
There was a problem hiding this comment.
Could using key be confusing in the context of a React component?
|
|
||
| const contextProviderValue = { | ||
| ToggleGroupControlOptionBase: { | ||
| isMultiple: true, |
Contributor
There was a problem hiding this comment.
Instead of using the context system, could we instead pass the isMultiple prop directly to the ToggleGroupControlOptionIcon inside ToggleMultipleGroupControlOptionIcon?
Maybe it could be a bit easier to discover for a developer? WDYT ? (not a strong opinion, though)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Extracted from #44168 (where a high-level review was completed first)
What?
Add a ToggleMultipleGroupControl component.
Why?
We need a component that looks like ToggleGroupControl, but allows multiple selection. Splitting this variant into a thin wrapper component keeps the API simpler. (See #44168 for details)
How?
Exports two new components — ToggleMultipleGroupControl and ToggleMultipleGroupControlOptionIcon — that are just thin wrappers around the original ToggleGroupControl components. Conditional logic is handled internally, and the API surface is kept simple.
From a DX standpoint, the main difference is that the consumer needs to manage the
isPressedstates on their own for each of the option buttons via anonClickhandler. This seems like more work, but it's actually a simpler DX compared to having the main ToggleMultipleGroupControl component handle all the states and centrally and trying to read them through a singleonChange.Testing Instructions
npm run storybook:devand check the docs/stories for ToggleMultipleGroupControl.✅ Unit tests pass
Screenshots or screencast