Skip to content

chore: add examples for aria labels for Input #1365

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

Merged
merged 14 commits into from
Jul 8, 2020

Conversation

rajasegar
Copy link
Contributor

Fixes #1353

@@ -21,6 +21,23 @@ Will become:
<label for="site">Ember Question</label>
<input id="site" type="text" value="How do text fields work?"/>
```
### Naming with aria-label
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe we want to remove this section and only show "naming with aria-labelledby. What do you think @MelSumner?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, that is my understanding as well and I agree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to have "ways to associate labels" and then use/explain all of them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little ping here for us to revisit this after we put up the Accessibility Spotlight blog posts perhaps. :)

Copy link
Contributor

@jenweber jenweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for your work on this @rajasegar! I've added some suggestions of my own, but suggest waiting to make changes until we hear from @MelSumner too.

Also, any time you use a technical property name like aria-labelledby outside of a code block, you can wrap it in backticks and the spellchecker will ignore it. The current test failures are just the spellchecker.


```handlebars
<span id="site-label">Ember Question</span>
<Input id="site" @value="How do text fields work?" aria-labelledby="site-label"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MelSumner Would it make sense for this to have both a regular label and a labelledby or describedby?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aria-labelledby is best used for search inputs. I'll suggest some changes to the example.

@rajasegar
Copy link
Contributor Author

Thanks a lot @jenweber for the suggestions. I didn't know about the spellchecker tests, I will keep that in mind next time.

@MelSumner MelSumner self-requested a review April 6, 2020 16:22

You can pass the following standard `<input>` attributes within the input
helper:
The `aria-label` property enables developers to name an element with a string that is not visually rendered.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue has some feedback on when to use property vs attribute in the prose. I mix them up all the time and I'm not sure which is correct in these examples.

Copy link
Member

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ship it and iterate more if we need to. @rajasegar can you move this out of draft status, please? Thanks for getting the ball rolling on this!

@rajasegar rajasegar changed the title WIP chore: add examples for aria labels for Input chore: add examples for aria labels for Input Apr 22, 2020
@rajasegar
Copy link
Contributor Author

Thanks Melanie, WIP status removed

Copy link
Member

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add some additional prose around line 51 that clarifies that HTML attributes such as required, disabled and readonly only need to be present and do not require a true or false value. Since it's confusing in general and extra confusing to JS devs, the clarity would help.

@rajasegar rajasegar marked this pull request as ready for review May 25, 2020 05:49
@jenweber
Copy link
Contributor

I would like to merge this and ask that contributors put further improvements in other PRs. Is that ok with everyone? I added a commit to fix some missing code fencing.

Thanks again y'all for your work on this!

@amyrlam amyrlam requested review from MelSumner and jenweber June 23, 2020 17:55
@amyrlam
Copy link
Member

amyrlam commented Jun 23, 2020

Looks ok to me, re-requested @MelSumner and @jenweber for review from talking to Isaac / looking at the history of the PR. Let me know if I can help!

@amyrlam amyrlam self-assigned this Jun 26, 2020
@ijlee2
Copy link
Member

ijlee2 commented Jul 8, 2020

Hi, everyone. I think this PR has sat out for too long. I read the changes once more to check that things read okay (good enough to allow further revisions if needed).

I think this PR was opened when release was v3.18, so I'm not sure if merging this will correctly update the current release, which is v3.19. If there's a mistake, I'll open a PR to fix it.

@ijlee2 ijlee2 merged commit 75ecf62 into ember-learn:master Jul 8, 2020
@ijlee2
Copy link
Member

ijlee2 commented Jul 8, 2020

Page is live! 🎉

@rajasegar Thank you for your work and patience in getting this PR across!

@rajasegar
Copy link
Contributor Author

Thanks @ijlee2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show example of aria-labelled by in Input section of the Guides
7 participants