ToggleGroupControl: Add de-selectable variant#45123
Merged
Conversation
|
|
34f56c4 to
12fd768
Compare
|
Size Change: +219 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
mirka
commented
Oct 19, 2022
Comment on lines
-69
to
-72
| } = { | ||
| ...toggleGroupControlContext, | ||
| ...buttonProps, | ||
| }; |
Member
Author
There was a problem hiding this comment.
Instead we're going to destructure our props from toggleGroupControlContext and buttonProps separately so we have better control over where they are forwarded.
| isActive && styles.buttonActive | ||
| styles.buttonView( { isDeselectable, isIcon, isPressed, size } ), | ||
| className | ||
| ); |
Member
Author
There was a problem hiding this comment.
The "icon" and "active" styles were moved to be handled in the buttonView function, because we can't control the cascade order in cx() itself.
Comment on lines
-63
to
-65
| export const separatorActive = css` | ||
| background: transparent; | ||
| `; |
Member
Author
There was a problem hiding this comment.
This wasn't being used anywhere 🧹
|
|
||
| function UnconnectedToggleGroupControl( | ||
| props: WordPressComponentProps< ToggleGroupControlProps, 'input', false >, | ||
| props: WordPressComponentProps< ToggleGroupControlProps, 'div', false >, |
Member
Author
There was a problem hiding this comment.
Apparently this was wrong. Rest props were being forwarded to a div, not an input.
| }; | ||
|
|
||
| return ( | ||
| <LabelView className={ labelViewClasses } data-active={ isActive }> |
Member
Author
There was a problem hiding this comment.
Removed the data-active because apparently this was only being used as a selector in an e2e test.
|
|
||
| await clickFontSizeButtonByLabel( 'Large' ); | ||
| const buttonSelector = `${ FONT_SIZE_TOGGLE_GROUP_SELECTOR }//div[@data-active='true']//button`; | ||
| const buttonSelector = `${ FONT_SIZE_TOGGLE_GROUP_SELECTOR }//button[@aria-checked='true']`; |
Member
Author
There was a problem hiding this comment.
Changed to a more standard selector.
This was referenced Oct 19, 2022
0338cae to
6f4b7a6
Compare
ciampo
reviewed
Oct 21, 2022
Contributor
ciampo
left a comment
There was a problem hiding this comment.
Thank you for splitting these changes in smaller PRs! It really improves the review process
I took a look at the new Storybook examples and things are looking pretty good!
Kapture.2022-10-21.at.13.36.33.mp4
Should we also remove the __experimentalIsBorderless prop from the Justification ToggleGroupControl ?
Comment on lines
-69
to
-72
| } = { | ||
| ...toggleGroupControlContext, | ||
| ...buttonProps, | ||
| }; |
packages/components/src/toggle-group-control/toggle-group-control-option-base/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control-option-base/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control-option-base/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control-option-base/styles.ts
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control/styles.ts
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
Show resolved
Hide resolved
- otherProps are forwarded to div, not `input` - ToggleGroupControl should not accept a `disabled` prop, but it was included via the FormElementProps type
- Removed unused `separatorActive` - Don't lighten button when deselectable
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
9588263 to
5591154
Compare
Member
Author
|
That was some great feedback, thanks! Everything should be addressed now.
Good catch, done 👍
|
ciampo
reviewed
Oct 27, 2022
| * | ||
| * @default false | ||
| */ | ||
| __experimentalIsBorderless?: boolean; |
Contributor
There was a problem hiding this comment.
Given the recent conversations around deprecating and removing props, are we able to simply delete this prop, even if it's experimental?
Member
Author
There was a problem hiding this comment.
Until any new rules are codified, I'd say yes. It also helps that this is a cosmetic difference that doesn't break layout.
packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx
Show resolved
Hide resolved
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
2 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 an
isDeselectableprop to allow ToggleGroupControl to be deselectable.Why?
See #44168
How?
isDeselectableprop is enabled, we use the button group implementation.__experimentalIsBorderlessprop is removed, since the border will be dictated by whether or not the control is deselectable.Testing Instructions
npm run storybook:devand see the ToggleGroupControl stories.Screenshots or screencast