Deprecating isPressed in Button component
#54740
Merged
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
andrewhayward
added
[Type] Enhancement
|
This change causes a big pile of tests to fail because of the new deprecation warning. Curious as to peoples thoughts about whether it's better to update the tests to expect the warning, or to just update the code to no longer use |
|
Size Change: +586 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 704770c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6276716510
|
Ideally, we should update the code to no longer use the deprecated prop, otherwise, folks will see the warning in their consoles. If it's too much work for a single PR, sometimes we migrate the props to the recommended alternative and only then deprecate the legacy prop. |
chad1008
reviewed
Sep 26, 2023
There was a problem hiding this comment.
This looks good and tests well for me. I see your point on the bug scope of consumers who've already added aria-pressed but not isPressed. After a quick search I don't see any cases of that in Gutenberg, so we're probably clear on that front at least internally, and can hope it's a minimal occurrence among third party consumers.
I think addressing the existing usages of isPressed as @tyxla suggested is the only real thing between this and a merge 👏 It looks like theres 30 ish of them to address, but they're not huge individual changes, so maybe a good fit for this PR? I'll defer to others if they disagree 😄
ciampo
approved these changes
Sep 26, 2023
There was a problem hiding this comment.
Looking good!
I think that refactoring other components can happen in one (or more) follow-up PRs.
I would also add the `aria-pressed` control to Storybook for completeness
diff --git a/packages/components/src/button/stories/index.story.tsx b/packages/components/src/button/stories/index.story.tsx
index a1dadab081..61b4c32438 100644
--- a/packages/components/src/button/stories/index.story.tsx
+++ b/packages/components/src/button/stories/index.story.tsx
@@ -26,6 +26,10 @@ const meta: Meta< typeof Button > = {
component: Button,
argTypes: {
// Overrides a limitation of the docgen interpreting our TS types for this as required.
+ 'aria-pressed': {
+ control: { type: 'select' },
+ options: [ undefined, 'true', 'false', 'mixed' ],
+ },
href: { type: { name: 'string', required: false } },
icon: {
control: { type: 'select' },
| @@ -131,7 +143,10 @@ export function UnforwardedButton( | |||
| 'is-small': size === 'small', | |||
| 'is-compact': size === 'compact', | |||
| 'is-tertiary': variant === 'tertiary', | |||
| 'is-pressed': isPressed, | |||
|
|
|||
| 'is-pressed': ariaPressed && ariaPressed !== 'false', | |||
There was a problem hiding this comment.
Could we simplify this check?
Suggested change
| 'is-pressed': ariaPressed && ariaPressed !== 'false', | |
| 'is-pressed': ariaPressed !== 'false', |
There was a problem hiding this comment.
The problem here is that ariaPressed has a boolean-ish value; it can either be true/"true", false/"false", "mixed" or undefined. So we can't just test for "false", because that would give false positives for false and undefined.
There was a problem hiding this comment.
If we wanted to be a bit more explicit, an alternative might be...
[true, "true", "mixed"].contains(ariaPressed)
There was a problem hiding this comment.
That variety of value possibilities is a good case for adding some unit tests, by the way.
There was a problem hiding this comment.
Thank you for the explanation, @andrewhayward ! For which specific implementation to use, I'll leave it up to you to decide which one you think is the clearest to understand for other developers coming across this component.
That variety of value possibilities is a good case for adding some unit tests, by the way.
I guess we could add a bunch of extra tests checking for false, 'true' and 'false' values too
There was a problem hiding this comment.
Added a few extra tests, and altered the logic around this to be a bit more explicit about what is expected.
ciampo
approved these changes
Sep 27, 2023
| @@ -131,7 +143,10 @@ export function UnforwardedButton( | |||
| 'is-small': size === 'small', | |||
| 'is-compact': size === 'compact', | |||
| 'is-tertiary': variant === 'tertiary', | |||
| 'is-pressed': isPressed, | |||
|
|
|||
| 'is-pressed': ariaPressed && ariaPressed !== 'false', | |||
There was a problem hiding this comment.
Thank you for the explanation, @andrewhayward ! For which specific implementation to use, I'll leave it up to you to decide which one you think is the clearest to understand for other developers coming across this component.
That variety of value possibilities is a good case for adding some unit tests, by the way.
I guess we could add a bunch of extra tests checking for false, 'true' and 'false' values too
andrewhayward
deleted the
enhancement/update-button-component-deprecate-ispressed
branch
ciampo
reviewed
Oct 3, 2023
Comment on lines
+311
to
+312
| &[aria-pressed="true"], | ||
| &[aria-pressed="mixed"] { |
There was a problem hiding this comment.
After seeing #55004, now I think about it, we could keep using the is-pressed class to assign those styles. Afterall, now the is-pressed class is assigned when truthyAriaPressedValues.includes( ariaPressed )
ciampo
added
the
Needs Dev Note
32 tasks
Dev noteThe In an effort to move away from custom props that have native HTML equivalents, and to allow greater flexibility in component usage, |
jeherve
added a commit
to Automattic/jetpack
that referenced
this pull request
Mar 11, 2024
The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740
3 tasks
jeherve
added a commit
to Automattic/jetpack
that referenced
this pull request
Apr 26, 2024
* Editor: remove deprecated isPressed prop in WP 6.5. The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740 * Update version * Bump version
matticbot
pushed a commit
to Automattic/jetpack-videopress
that referenced
this pull request
Apr 26, 2024
* Editor: remove deprecated isPressed prop in WP 6.5. The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740 * Update version * Bump version Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/8847567798 Upstream-Ref: Automattic/jetpack@adace6c
matticbot
pushed a commit
to Automattic/jetpack-storybook
that referenced
this pull request
Apr 26, 2024
* Editor: remove deprecated isPressed prop in WP 6.5. The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740 * Update version * Bump version Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/8847567798 Upstream-Ref: Automattic/jetpack@adace6c
matticbot
pushed a commit
to Automattic/jetpack-backup-plugin
that referenced
this pull request
Apr 26, 2024
* Editor: remove deprecated isPressed prop in WP 6.5. The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740 * Update version * Bump version Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/8847567798 Upstream-Ref: Automattic/jetpack@adace6c
matticbot
pushed a commit
to Automattic/jetpack-videopress-plugin
that referenced
this pull request
Apr 26, 2024
* Editor: remove deprecated isPressed prop in WP 6.5. The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740 * Update version * Bump version Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/8847567798 Upstream-Ref: Automattic/jetpack@adace6c
matticbot
pushed a commit
to Automattic/jetpack-starter-plugin
that referenced
this pull request
Apr 26, 2024
* Editor: remove deprecated isPressed prop in WP 6.5. The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740 * Update version * Bump version Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/8847567798 Upstream-Ref: Automattic/jetpack@adace6c
matticbot
pushed a commit
to Automattic/wpcom-migration
that referenced
this pull request
Apr 26, 2024
* Editor: remove deprecated isPressed prop in WP 6.5. The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740 * Update version * Bump version Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/8847567798 Upstream-Ref: Automattic/jetpack@adace6c
matticbot
pushed a commit
to Automattic/jetpack-protect-plugin
that referenced
this pull request
Apr 26, 2024
* Editor: remove deprecated isPressed prop in WP 6.5. The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740 * Update version * Bump version Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/8847567798 Upstream-Ref: Automattic/jetpack@adace6c
matticbot
pushed a commit
to Automattic/jetpack-boost-production
that referenced
this pull request
Apr 26, 2024
* Editor: remove deprecated isPressed prop in WP 6.5. The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740 * Update version * Bump version Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/8847567798 Upstream-Ref: Automattic/jetpack@adace6c
matticbot
pushed a commit
to Automattic/jetpack-social-plugin
that referenced
this pull request
Apr 26, 2024
* Editor: remove deprecated isPressed prop in WP 6.5. The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740 * Update version * Bump version Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/8847567798 Upstream-Ref: Automattic/jetpack@adace6c
matticbot
pushed a commit
to Automattic/jetpack-search-plugin
that referenced
this pull request
Apr 26, 2024
* Editor: remove deprecated isPressed prop in WP 6.5. The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740 * Update version * Bump version Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/8847567798 Upstream-Ref: Automattic/jetpack@adace6c
matticbot
pushed a commit
to Automattic/jetpack-production
that referenced
this pull request
Apr 26, 2024
* Editor: remove deprecated isPressed prop in WP 6.5. The isPressed prop is deprecated in WP 6.5, and replaced by the native aria-pressed attribute. WordPress/gutenberg#54740 * Update version * Bump version Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/8847567798 Upstream-Ref: Automattic/jetpack@adace6c
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?
This PR starts the deprecation of
isPressedon theButtoncomponent, deferring to the nativearia-pressedattribute instead. While it does not externally change the API, internally it convertsisPressedintoaria-pressed, such that using the latter is equivalent to the former. As such,aria-pressedcan be used instead ofisPressedshould a consumer so desire.Why?
When the
isPressedprop is used onButton,aria-pressedis explicitly set accordingly. But sometimesButtonis used for roles other thanbutton, such asoptionandcheckbox, wherearia-pressedis not appropriate. Instead consumers should be able to decide for themselves the appropriate semantics.How?
The
isPressedprop is marked as deprecated, and transformed intoaria-pressed. The value ofaria-pressedis then used instead to determine how the component should be styled.Testing Instructions
Unit tests have been added to confirm that the legacy
isPressedbehaviour continues, and that usingaria-presseddirectly behaves in the same way.The only potential scope for issues is where a consumer has already set
aria-pressedon aButton, but notisPressed, as this could affect the visual appearance due to the addedis-pressedclass.