-
Notifications
You must be signed in to change notification settings - Fork 6.8k
demo(list): Add accessibility demo page for list #6363
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
Conversation
| <section> | ||
| <h2>Mailbox navigation</h2> | ||
| <p>Showing a navigation list with links to google search, and "more information" icon button</p> | ||
| <md-nav-list> |
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.
md-nav-list is only meant to contain <a md-list-item> children.
This makes me realize that we probably need tweaks as part of this:
- Removing the existing roles and saying that, by default,
md-listis assumed to be presentational, and add instructions to add the roles for non-interactive content (same as grid-list). - Give
md-nav-listrole="navigation"
Thoughts?
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.
- Made another component
MdNavListformat-nav-listwithrole="navigation" - Removed
role="list"forMdList - Added "accessibility" section in list.md (Same as grid-list)
2b1e3f5 to
66b88c2
Compare
kara
left a comment
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.
LGTM, one nit
src/lib/list/list.md
Outdated
| ### Accessibility | ||
| By default, the list assumes that it will be used in a purely decorative fashion and thus sets no | ||
| roles, ARIA attributes, or keyboard shortcuts. | ||
| This is equivalent to having a sequence of <div> elements on the page. Any interactive content |
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.
Nit: Missing new line before this paragraph starts?
| constructor(private _renderer: Renderer2, | ||
| private _element: ElementRef, | ||
| @Optional() private _list: MdList, | ||
| @Optional() navList: MdNavListCssMatStyler) { |
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.
Are we still using MdNavListCssMatStyler for anything or can we remove it now?
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.
LGTM, a few nits.
| </section> | ||
|
|
||
| <section> | ||
| <h2> Seasoning</h2> |
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.
Spacing inside the tag seems uneven.
| </section> | ||
|
|
||
| <section> | ||
| <h2>Messages </h2> |
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.
Uneven spacing here as well.
src/demo-app/a11y/list/list-a11y.ts
Outdated
| } | ||
| ]; | ||
|
|
||
| links: any[] = [ |
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.
If you remove the any[] from these arrays, TS should be able to figure out the proper type.
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.
LGTM
|
Thanks for review. Removed |
src/lib/list/list.md
Outdated
| ### Accessibility | ||
| By default, the list assumes that it will be used in a purely decorative fashion and thus sets no | ||
| roles, ARIA attributes, or keyboard shortcuts. This is equivalent to having a sequence of <div> | ||
| elements on the page. Any interactive content within the grid-list should be given an appropriate |
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 line still says grid-list
src/lib/list/list.md
Outdated
| accessibility treatment based on the specific workflow of your application. | ||
|
|
||
| If the list is used to present a list of non-interactive content items, then the list element should | ||
| be given `role="list"` and each tile should be given `role="listitem"`. |
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.
tile -> list item
|
Comments addressed. PTAL. Thanks! |
jelbourn
left a comment
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.
LGTM
|
@tinayuangao There looks like there might be a real unit test failure for list here: https://travis-ci.org/angular/material2/jobs/265757068 Can you take a look? |
cec0398 to
65f82c3
Compare
f6cdcf0 to
44b53bf
Compare
1453d17 to
8e2af31
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.