Skip to content

Refresh SelectMenu#1064

Merged
simurai merged 7 commits intonextfrom
next-select-menu
Apr 15, 2020
Merged

Refresh SelectMenu#1064
simurai merged 7 commits intonextfrom
next-select-menu

Conversation

@simurai
Copy link
Contributor

@simurai simurai commented Apr 9, 2020

This refreshes the SelectMenu 👀 Preview. Changes include:

  • The header is now optional. But show the close button also on desktop.
  • Everything has now a white background.
  • The filter and tabs can be switched in order.
  • The height is mostly kept at 32px (excluding the border) to match other components.
  • Add option to remove the borders in the list
  • Add option to disable a list item
Current Next Refreshed (this PR)
image Screen Shot 2020-04-09 at 3 51 50 PM Screen Shot 2020-04-09 at 3 51 13 PM

API changes

  • Add .SelectMenu-list--borderless modifier
  • Add support for disabled + aria-disabled="true" list items

/cc @primer/ds-core

@vercel
Copy link

vercel bot commented Apr 9, 2020

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/567bgohgv
✅ Preview: https://primer-css-git-next-select-menu.primer.now.sh

@simurai
Copy link
Contributor Author

simurai commented Apr 9, 2020

We could also consider removing the borders in the list?

no-border

Not sure about being the default? For multiline items, having a border in between is probably still needed. But maybe ok to make single line items feel more lightweight?

@simurai
Copy link
Contributor Author

simurai commented Apr 9, 2020

Here some screenshots with switching the order of the filter and tabs:

Global filter Per tab filter
Screen Shot 2020-04-09 at 4 11 57 PM Screen Shot 2020-04-09 at 4 12 21 PM

So if the filter should be applied to both tabs, it should be at the top. Or if a filter should only work for the content inside a tab, the filter can be moved to after the tabs.

Not sure if that will work on dotcom and if Primer Components should support that too?

/cc @ashygee @TheDamianHdez @emplums

@simurai simurai marked this pull request as ready for review April 9, 2020 07:45
@simurai simurai requested review from auareyou and removed request for auareyou April 9, 2020 07:45
@simurai simurai marked this pull request as draft April 9, 2020 08:50
@vercel vercel bot temporarily deployed to Preview April 13, 2020 14:27 Inactive
@vercel vercel bot temporarily deployed to Preview April 14, 2020 04:34 Inactive
@simurai
Copy link
Contributor Author

simurai commented Apr 14, 2020

There is now a "borderless" option by adding .SelectMenu-list--borderless:

borderless

This also works with dividers. You can use it with text like <div class="SelectMenu-divider">More options</div> or more like a visual separate like <hr class="SelectMenu-divider">:

Default .SelectMenu-list--borderless
Screen Shot 2020-04-14 at 1 36 38 PM Screen Shot 2020-04-14 at 1 36 51 PM

So adding .SelectMenu-list--borderless and using a <hr class="SelectMenu-divider"> would just add a single line/border between items.

@vercel vercel bot temporarily deployed to Preview April 14, 2020 05:35 Inactive
@simurai
Copy link
Contributor Author

simurai commented Apr 14, 2020

There is now a disabled state for list items:

disabled

Markup:

<button class="SelectMenu-item">Item 1</button>
<button class="SelectMenu-item" disabled>Item 2 (disabled)</button>
<a class="SelectMenu-item" href="#">Item 3</a>
<a class="SelectMenu-item" aria-disabled="true">Item 4 (disabled)</a>

There was some back and forth about having a disabled state for list items. In the docs there is the following note: "If not obvious, try to communicate to the user why an item is disabled."

@simurai simurai marked this pull request as ready for review April 14, 2020 05:39
@simurai simurai requested a review from auareyou April 14, 2020 05:39
Copy link
Contributor

@auareyou auareyou left a comment

Choose a reason for hiding this comment

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

Is the close icon now included on all versions or is it optional?

@auareyou auareyou self-requested a review April 14, 2020 08:42
Copy link
Contributor

@auareyou auareyou left a comment

Choose a reason for hiding this comment

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

🙌

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.

2 participants