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

SelectMenu icon improvements #965

Closed
wants to merge 3 commits into from

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Oct 30, 2019

This is on top of #922 and adds even more improvements. YAY :zany:

1. 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.

2. 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".

Was also wondering if we need some kind of SelectMenu--truncate. Similar to css-truncate but without the need for overriding max-width. Maybe in another PR as a utility.

/cc @muan


Closes #961

@vercel
Copy link

vercel bot commented Oct 30, 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/4g961gunj
🌍 Preview: https://primer-css-git-select-menu-icon.primer.now.sh

@simurai simurai mentioned this pull request Oct 30, 2019
7 tasks
@simurai simurai requested a review from shawnbot November 7, 2019 01:39
@shawnbot shawnbot mentioned this pull request Nov 15, 2019
19 tasks
@simurai simurai force-pushed the more-select-menu-improvements branch from af24fe6 to 2c8afda Compare November 15, 2019 13:47
@simurai
Copy link
Contributor Author

simurai commented Nov 15, 2019

Closing since this PR is now part of #922.

@simurai simurai closed this Nov 15, 2019
@simurai simurai deleted the select-menu-icon branch November 15, 2019 13:52
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.

1 participant