-
Notifications
You must be signed in to change notification settings - Fork 205
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
feat(field-label): add field label pattern #1031
Conversation
Clicking the label should move focus to the associated form element. This works fine for the sp-textfield but doesn't seem to work for the sp-dropdown |
@jnurthen thanks for checking in. Can you confirm a couple of things for me?
If yes to the spacebar, then this has to do with "click" not triggering the heuristic for |
This occurred on both Safari & Chrome (Current production versions) on MacOS 10.15.7 react-spectrum ( https://react-spectrum.adobe.com/react-spectrum/Picker.html ) does draw the focus ring in this case and I suggest that SWC should do the same. |
Haha. Gotta love the way the web has to motivate entire swaths of designers, devs and product owners to get an accessible feature into the platform only to work around its presence later. Thanks for the clarification. I'll get that worked out and added to the PR. I'll still bring this up with Spectrum design/CSS as the |
77b3215
to
55a6e77
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 don't have time to git this a really deep review, but this looks good overall.
I've got one comment that could be improved now (or later). I don't think you need to block this on it though.
if (!this.hasAttribute('id')) { | ||
this.setAttribute( | ||
'id', | ||
`sp-field-label-${FieldLabel.instanceCount++}` |
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.
this is the sort of code that will cause subtle conflicts if multiple sp-field-label
classes can be instantiated, or if this is subclassed. Can you use the tag name directly here? That'd at least give some safety here.
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 idea. Updated!
38ec0fd
to
cb744a1
Compare
Expanded on the tests here, and really hope we can get this reviewed/merged soon! |
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.
Looks good to me!
cb744a1
to
eb34a45
Compare
Description
Preview available at: https://5fc853af5b61815cd54df3fd--spectrum-web-components.netlify.app
Related Issue
fixed #475
Motivation and Context
Labels are an important accessibility and usability tool.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: