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

More SelectMenu improvements #922

Merged
merged 5 commits into from
Nov 20, 2019
Merged

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Oct 1, 2019

This adds a few more improvements to the .SelectMenu .

TODO:

  • Fix top border
  • Increase max height
  • Introduce .SelectMenu-icon--check
  • Prevent shrinking of the icon
  • Deprecate the removed selectors

TODO on dotcom:


While testing the SelectMenu for the "branch picker" more issues came up:

1. When filtering, a top border is visible

When using fuzzy-list to filter, the DOM nodes get removed, but for the branch picker display: none is used. This has the effect that :first/:last-child can't be used anymore.

Fix: Use a bottom-border and then margin-bottom: -1px on the list to hide the "last" border fb3a10a

Using box-shadow would be an alternative because it gets cut off within overflow-y: auto;. But it seems scrolling performance suffers.

Before After
Before After

2. The branch list is not as tall

Fix: Increase max-height a bit. 279c87e

Before After
Before After

3. Introduce .SelectMenu-icon--check

Currently we use the SelectMenu-icon class specifically for the check icon to show if an item is selected or not. But there have been cases where a SelectMenu uses different icons unrelated to the selected/checked state. This PR:

  • Makes the .SelectMenu-icon more generic. It only cares about the size and margin and can be used for any icon.
  • Adds an additional .SelectMenu-icon--check modifier class that toggles based on the aria-checked="true" attribute.
<button class="SelectMenu-item" role="menuitemcheckbox" aria-checked="true">
  <%= octicon "check", class: "SelectMenu-icon SelectMenu-icon--check" %>
  Selectable item with a check icon
</button>
<button class="SelectMenu-item" role="menuitem">
  <%= octicon "pin", class: "SelectMenu-icon" %>
  Item with a pin icon
</button>

Above markup renders as:

image

It allows to mix check and other icons in the same list and have them aligned without fiddling with margin utilities.

Alternatives

Some alternative names instead of SelectMenu-icon--check:

  • .SelectMenu-icon--checked might be confused for adding a state
  • .SelectMenu-icon--selected same
  • .SelectMenu--showWhenChecked could be used for anything, not just icons. People might expect it to be display: none by default, instead of visibility: hidden. But that would make the text jump.

4. Prevent shrinking of the icon

Happens when the text starts to wrap, see #961. A flex-shrink-0 can be used, but might be convenient to "bake that in".

@vercel
Copy link

vercel bot commented Oct 1, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/primer/primer-css/1uyech6h7
🌍 Preview: https://primer-css-git-more-select-menu-improvements.primer.now.sh

@simurai simurai mentioned this pull request Oct 1, 2019
10 tasks
@shawnbot shawnbot mentioned this pull request Oct 1, 2019
19 tasks
@shawnbot shawnbot changed the base branch from master to release-14.0.0 October 22, 2019 18:51
@simurai
Copy link
Contributor Author

simurai commented Nov 15, 2019

FYI: While resolving merge conflicts, I also added the changes from #965 into this branch.

I think this should now be ready.

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.

This looks great. Thanks for working through all these issues!

@simurai simurai merged commit 7c76bc4 into release-14.0.0 Nov 20, 2019
@simurai simurai deleted the more-select-menu-improvements branch November 20, 2019 15:29
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