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

Better keyboard support for distributor admin menu #569

Merged
merged 8 commits into from
May 19, 2020

Conversation

samikeijonen
Copy link
Contributor

@samikeijonen samikeijonen commented Mar 26, 2020

Fixes #563

Description of the Change

  • Use <button> instead of <div>.
  • Use Core button styles where possible for consistency and better focus styles.
  • Use disabled attribute when post have been syndicated.
  • This is not full refactoring but first step in better accessibility.

Alternate Designs

None.

Benefits

Better keyboard support when using correct semantics out of the box.

Possible Drawbacks

Complex codebase, needs testing that nothing is broken.

Verification Process

Distribute and push article using only keyboard.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

#563

@jeffpaul jeffpaul requested a review from dinhtungdu March 26, 2020 13:42
@jeffpaul jeffpaul added the type:enhancement New feature or request. label Mar 26, 2020
@jeffpaul jeffpaul added this to the 1.6.0 milestone May 2, 2020
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

The code looks good @samikeijonen. There is a small issue that internal connections can't be selected using keyboard because they're still using div. Can you please update?

height: 30px;
line-height: 28px;
padding: 0 12px 2px;
border-radius: 3px;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should keep the border-radius and text-shadow because they're overridden by admin bar style.

@samikeijonen
Copy link
Contributor Author

@dinhtungdu Unfortunately I have zero hours for updates at the moment.

@dinhtungdu
Copy link
Contributor

@samikeijonen no problem, I will take over this.

@dinhtungdu dinhtungdu self-requested a review May 15, 2020 03:33
dinhtungdu
dinhtungdu previously approved these changes May 15, 2020
@jeffpaul jeffpaul requested a review from dkotter May 15, 2020 04:51
@dkotter
Copy link
Collaborator

dkotter commented May 15, 2020

@dinhtungdu Testing this locally, I'm seeing the connection list break if you have more than 4 connections:

Screen Shot 2020-05-15 at 11 28 51 AM

I believe once there are 5 or more connections, we constrain those to a box that you can scroll through, to keep the list down. Looks like something changed here that is preventing that from working properly, so the entire connection list is shown and there's no way to scroll through them all.

@dinhtungdu
Copy link
Contributor

Thank @dkotter, you're right, the style to handle overflow was removed. I added it again, the list is now working correctly with more than 4 connections.

@dkotter
Copy link
Collaborator

dkotter commented May 18, 2020

@dinhtungdu Looks good 👍

@jeffpaul jeffpaul merged commit 704baea into 10up:develop May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distributor menu in admin bar - better keyboard support
4 participants