Add accessibility page to demo-app #5748
Conversation
googlebot
added
the
cla: yes
| @@ -0,0 +1,36 @@ | |||
| <md-sidenav-container class="demo-root" fullscreen> | |||
There was a problem hiding this comment.
For the a11y demo, I think it would be better to omit the sidenav and just iterate over the <a> elements inside a <nav> at the top of the page. Not pretty, but simple. The sidenav will potentially just complicate the a11y testing, since we want each page to be totally focused on the component under test.
We can also consider adding a skip link at the beginning
There was a problem hiding this comment.
Changed to <nav> with <a> but "skip link" seems not working angular/angular#6595
| <button md-button (click)="start.close()">CLOSE</button> | ||
| </md-sidenav> | ||
| <div> | ||
| <md-toolbar color="primary"> |
There was a problem hiding this comment.
I would omit this toolbar and everything in it. These controls are useful for development but are just noise for a11y testing.
Instead of the toolbar we should just have a single <h1> with the title of the specific demo.
| </div> | ||
| </md-toolbar> | ||
|
|
||
| <div #root="dir" dir="ltr" class="demo-content" > |
| selector: 'accessibility-demo', | ||
| templateUrl: './accessibility.html', | ||
| host: { | ||
| '[class.unicorn-dark-theme]': 'dark', |
There was a problem hiding this comment.
Don't need a theme for a11y testing
|
|
||
| constructor(private _element: ElementRef) {} | ||
|
|
||
| toggleFullscreen() { |
There was a problem hiding this comment.
Don't need the fullscreen stuff
| <section> | ||
| <button md-button>Check</button> | ||
| <button md-raised-button>raised</button> | ||
| <button md-fab> |
There was a problem hiding this comment.
Buttons that just contain icons need an aria-label:
<button md-fab aria-label="Activate floating action style button">
<md-icon class="md-24">check</md-icon>
</button>
<button md-mini-fab aria-label="Activate mini floating action style button"> ... </button>| @@ -0,0 +1,23 @@ | |||
| <div class="demo-button"> | |||
There was a problem hiding this comment.
I would name the files button-a11y.xx for brevity
| <button md-mini-fab> | ||
| <md-icon class="md-24">check</md-icon> | ||
| </button> | ||
| </section> |
There was a problem hiding this comment.
Also need an example of md-icon-button
| </section> | ||
|
|
||
| <section> | ||
| <a href="http://www.google.com" md-button color="primary">SEARCH</a> |
There was a problem hiding this comment.
Avoid using ALL_CAPS directly because screen-readers will read them out as "S - E - A - R - C - H".
This one should be, e.g., "Google search".
| <section> | ||
| <a href="http://www.google.com" md-button color="primary">SEARCH</a> | ||
| <a href="http://www.google.com" md-raised-button>SEARCH</a> | ||
| <a href="http://www.google.com" md-fab> |
There was a problem hiding this comment.
Same thing about aria-label here
tinayuangao
force-pushed
the
a18n-site
branch
2 times, most recently
from
bf6e99d to
7367ac0
Compare
|
Comments addressed. Please take another look. Thanks! |
src/demo-app/a11y/a11y.html
Outdated
|
|
||
| <nav> | ||
| <ul> | ||
| <li *ngFor="let navItem of navItems"> |
There was a problem hiding this comment.
You don't need the ul / li; the <a> should just be a child of the nav.
src/demo-app/a11y/a11y.ts
Outdated
| moduleId: module.id, | ||
| selector: 'accessibility-home', | ||
| template: `<p>Welcome to the accessibility demos for Angular Material!</p> | ||
| <p>Open the sidenav to select a demo.</p>`, |
There was a problem hiding this comment.
Template still mentions the sidenav
| @@ -0,0 +1,30 @@ | |||
| <div class="demo-button"> | |||
| <section> | |||
| <h2></h2> | |||
There was a problem hiding this comment.
h2 is empty. I'd make the header Button elements
| </section> | ||
|
|
||
| <section> | ||
| <a href="http://www.google.com" md-button color="primary">Google search</a> |
There was a problem hiding this comment.
Add <h2> here; I'd make the heading Anchor elements
| template: '<router-outlet></router-outlet>', | ||
| encapsulation: ViewEncapsulation.None, | ||
| }) | ||
| export class EntryApp {} |
There was a problem hiding this comment.
Add comment for how this is used?
tinayuangao
force-pushed
the
a18n-site
branch
3 times, most recently
from
c1f68d6 to
364486a
Compare
|
Comments addressed. Please take another look. Thanks! |
jelbourn
added
pr: lgtm
action: merge
|
Please rebase |
|
Rebased |
|
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. |
/accessibilitybuttonaccessibility demo page (placeholder)