Skip to content
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 add focus management for toolbar #30

Merged
merged 28 commits into from
Apr 2, 2020

Conversation

keithamus
Copy link
Member

This follows best practices for toolbar focus management, following the guidelines available at https://www.w3.org/TR/wai-aria-practices-1.1/examples/toolbar/toolbar.html.

ArrowLeft/ArrowRight will switch tabindex/focus the next and previous buttons respectively, cycling round the whole toolbar if needed. Home and End keys were also implemented.

Refs https://github.com/github/github/issues/133916

This follows best practices for toolbar focus management, following the
guidelines available at https://www.w3.org/TR/wai-aria-practices-1.1/examples/toolbar/toolbar.html.

ArrowLeft/ArrowRight will switch tabindex/focus the next and previous
buttons respectively, cycling round the whole toolbar if needed. Home
and End keys were also implemented.
@keithamus keithamus requested a review from a team March 17, 2020 14:56
keithamus and others added 3 commits March 18, 2020 09:50
Co-Authored-By: Mu-An 慕安 <me@muanchiou.com>
This allows non-MarkdownButtonElement instances to be part of the focus
management cycle
This allows for non-MarkdownButtonElement members to also handle focus
management keyboard shortcuts, as the event binding is per-toolbar not
per-button
This catches an edge case where if the `md-*` buttons aren't upgraded
before `markdown-toolbar` is, then `markdown-toolbar` will still behave
correctly, as it can still select for the `md-*` buttons.
Now markdown-toolbar manages all tabindex assignment, which means it
will also assign tabindexes to `data-md-button` elements, as needed.
Copy link
Contributor

@koddsson koddsson left a comment

Choose a reason for hiding this comment

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

This looks good to me, does what it says in the description.

Screen Recording 2020-03-25 at 11 07 34

Copy link
Contributor

@muan muan left a comment

Choose a reason for hiding this comment

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

For posterity: @keithamus and I paired this and noticed that we need a proper visibility check on the buttons for the responsive markdown toolbar items.

muan
muan previously approved these changes Mar 30, 2020
Copy link
Contributor

@muan muan left a comment

Choose a reason for hiding this comment

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

Looks good! Only missing some documentation now in the README. A link to the spec and some description on how to add custom buttons in to the navigation sequence should suffice.

Found an issue.

@muan muan self-requested a review March 30, 2020 21:02
@muan muan dismissed their stale review March 30, 2020 21:55

Found an issue.

This allows toolbars to be in an initially hidden state, and only when
they are visible and focus moves to them does the focus management kick
in.
Copy link
Contributor

@muan muan left a comment

Choose a reason for hiding this comment

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

👍🏻 Still needs docs.

keithamus and others added 4 commits April 1, 2020 10:56
Co-Authored-By: Mu-An 慕安 <me@muanchiou.com>
Co-Authored-By: Mu-An 慕安 <me@muanchiou.com>
Co-Authored-By: Mu-An 慕安 <me@muanchiou.com>
@keithamus keithamus requested a review from muan April 1, 2020 11:25
Copy link
Contributor

@muan muan left a comment

Choose a reason for hiding this comment

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

👍 !

Co-Authored-By: Mu-An 慕安 <me@muanchiou.com>
@keithamus keithamus merged commit f5afa6b into master Apr 2, 2020
@keithamus keithamus deleted the feat-add-focus-management-for-toolbar branch August 3, 2020 14:49
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.

4 participants