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

Select Menu improvements #893

Merged
merged 3 commits into from
Sep 12, 2019
Merged

Select Menu improvements #893

merged 3 commits into from
Sep 12, 2019

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Sep 11, 2019

Here some follow-up improvements/tweaks that got discovered while using the .SelectMenu on dotcom: https://github.com/github/design-systems/issues/666

@vercel
Copy link

vercel bot commented Sep 11, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://primer-css-git-select-menu-improvements.primer.now.sh

@simurai
Copy link
Contributor Author

simurai commented Sep 11, 2019

Original comment by @muan https://github.com/github/github/pull/116285#discussion_r322834653

We shouldn't use <menu> as it has poor support across browsers. It provides no semantic value in all major browsers while expecting only <li> children. This introduces unnecessary markup constraint and breaks the experience for screen reader users. (See reports in firefox, webkit, chrome).
Also, <details-menu> already sets role=menu on itself if that's what we are looking for.

Initially I thought using <menu> is a good way to make it easier to read the markup. So that you see where the SelectMenu-list "starts" and "ends". This PR changes the documentation to use <div>s instead.

@simurai
Copy link
Contributor Author

simurai commented Sep 11, 2019

When switching tabs, the text "jumps" by 1px:

branch

In this PR it gets fixed by permanently adding a transparent border and then only change the border-color when a tab is selected.

Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

Nice, thanks! Let's include this in #887.

@shawnbot shawnbot changed the base branch from master to release-13.0.0 September 11, 2019 15:30
@shawnbot shawnbot mentioned this pull request Sep 11, 2019
15 tasks
@simurai simurai marked this pull request as ready for review September 12, 2019 11:14
@simurai simurai merged commit 3533fe4 into release-13.0.0 Sep 12, 2019
@simurai simurai deleted the select-menu-improvements branch September 12, 2019 11:15
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.

2 participants