-
Notifications
You must be signed in to change notification settings - Fork 2k
Experience Control: Improve accessibility and minor refactoring #103000
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~200 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great details! Ty, @yashwin ! Lgtm!
Thanks for the quick follow-up! I'm clarifying a few things first in the Figma thread (dhW7zYBamXJIeJIE3wy5f4-fi-111_846#1193979809) since this is the first time I'm seeing this discussion. |
6687baf
to
d4e1eca
Compare
Thanks @mirka ! I think that's enough, as the component is already imported here:
Changelog doesn't mention |
e2c4506
to
b49d4a8
Compare
b49d4a8
to
3229a5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some minor thoughts about implementation, but the accessibility issues are fixed 🎉
None of the notes are blockers, but we should probably fix the Storybook controlled mode issue since the stories currently look broken.
<label className={ clsx( 'a8c-experience-control__option', className ) }> | ||
<input | ||
type="radio" | ||
className="a8c-experience-control__radio" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CSS is being written to hide this visually, but that can be done with a component like VisuallyHidden
.
<label className={ clsx( 'a8c-experience-control__option', className ) }> | ||
<input | ||
type="radio" | ||
className="a8c-experience-control__radio" | ||
aria-label={ ariaLabel } | ||
{ ...restProps } | ||
/> | ||
<div className="a8c-experience-control__option-label">{ label }</div> | ||
</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of label
and aria-label
here are a bit unclear to me. label
implies a string, and it's non-obvious whether label
or aria-label
is going to take precedence as the actual label of the input. Everything being wrapped in a <label>
element also strongly implies that label
is expected to be a text node, or at least have a text representation.
I can think of a few ways around this weirdness, for example we can expect the consumer to pass in an already accessibly-labeled label
node, instead of a separate aria label that is passed on to the input
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
Do you think we can use icon
instead of label
something like:
const ExperienceControlOption = ( {
className,
icon,
ariaLabel,
...restProps
}: ExperienceControlOptionProps ) => (
<label className={ clsx( 'a8c-experience-control__option', className ) }>
<VisuallyHidden>
<input type="radio" aria-label={ ariaLabel } { ...restProps } />
</VisuallyHidden>
<div className="a8c-experience-control__option-icon" aria-hidden="true">
{ icon }
</div>
</label>
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that's probably fine for now, since we only have this one use case. In the future when this abstraction needs to support more use cases, it may be too limiting since it won't necessarily be an icon (e.g. an NPS selector where it's just plain text numbers). But we don't have to overthink it at this moment.
Thanks for the review, @mirka! Do you think we can now remove it from the |
Yes, go ahead 👍 |
3229a5b
to
d4ff835
Compare
I pushed a fix for the Storybook crashes. Looks like something changed in a recent update. |
Closes https://github.com/Automattic/automattic-for-agencies-dev/issues/2112
Proposed Changes
This PR addressed the comments we received from @mirka on this PR that implemented the Experience Control on the Automattic package. We copied the initial implementation from the original PR, which wasn't accessibility-friendly. This PR addresses that issue.
Why are these changes being made?
Testing Instructions
Untitled.mov
Pre-merge Checklist