Skip to content

Make 'time picker' button in Discover no results prompt keyboard and screen-reader accessible.#13046

Merged
cjcenizal merged 3 commits intoelastic:masterfrom
cjcenizal:12970/local-nav-aria
Jul 26, 2017
Merged

Make 'time picker' button in Discover no results prompt keyboard and screen-reader accessible.#13046
cjcenizal merged 3 commits intoelastic:masterfrom
cjcenizal:12970/local-nav-aria

Conversation

@cjcenizal
Copy link
Copy Markdown
Contributor

Fixes #12970

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's exactly the same label as the content currently is, so we can just remove this aria-label.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You didn't introduce this here, but I feel like nitpicking about it now (though no need to fix it) ;P
This should actually be a button and not an a imho, since it "does" something on the current page and doesn't navigate to another page.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great call!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since this link doesn't have an href attribute it wouldn't get the link role by default so we should add this role manually.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess you already discussed this, but can't find it right now: wouldn't it make sense to add ngAria as a dependency, to solve that basic issues (like automatically make ng-click stuff tabable and clickable by keyboard)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would you mind creating a new issue for this, with the "discuss" and "Accessibility" labels? This is an interesting idea and it seems like it would solve a good set of low-level issues! One tradeoff is that it's magic and relives the engineer of the responsibility of understanding and using ARIA well, which could be problematic when migrating Angular code to React.

@cjcenizal
Copy link
Copy Markdown
Contributor Author

@timroes Changing the element type to button seems to address all of your feedback. Mind taking another look?

Copy link
Copy Markdown
Contributor

@bhavyarm bhavyarm left a comment

Choose a reason for hiding this comment

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

LGTM

@bhavyarm
Copy link
Copy Markdown
Contributor

@cjcenizal all good. I don't see that severe warning anymore. Cheers!

@cjcenizal cjcenizal force-pushed the 12970/local-nav-aria branch from d12e70a to 77cb9de Compare July 26, 2017 15:08
@cjcenizal cjcenizal merged commit 1675529 into elastic:master Jul 26, 2017
@cjcenizal cjcenizal deleted the 12970/local-nav-aria branch July 26, 2017 23:24
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Jul 26, 2017
…screen-reader accessible. (elastic#13046)

* Make 'time picker' button in Discover no results prompt keyboard and screen-reader accessible.
cjcenizal added a commit that referenced this pull request Jul 26, 2017
…screen-reader accessible. (#13046) (#13134)

* Make 'time picker' button in Discover no results prompt keyboard and screen-reader accessible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants