-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adjust Select selected options UI #1659
Conversation
2295acc
to
2f21ab2
Compare
@@ -99,11 +99,10 @@ function SelectOption<T>({ | |||
<li | |||
className={classnames( | |||
'w-full ring-inset outline-none rounded-none select-none', | |||
'border-t first:border-t-0 whitespace-nowrap', | |||
'px-1 mb-1 first:mt-1 whitespace-nowrap group', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have decided to implement spacing using margins and padding on the options, so that it's easier to eventually include the search boxes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, but can you clarify how this makes adding the search boxes easier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1659 +/- ##
===========================================
+ Coverage 0 100.00% +100.00%
===========================================
Files 0 61 +61
Lines 0 1040 +1040
Branches 0 402 +402
===========================================
+ Hits 0 1040 +1040 ☔ View full report in Codecov by Sentry. |
Yes. Some small tweaks to the item layout in that example would solve the problem. |
2f21ab2
to
84d7b54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The check definitely seems more visually obvious than the red bar.
@@ -99,11 +99,10 @@ function SelectOption<T>({ | |||
<li | |||
className={classnames( | |||
'w-full ring-inset outline-none rounded-none select-none', | |||
'border-t first:border-t-0 whitespace-nowrap', | |||
'px-1 mb-1 first:mt-1 whitespace-nowrap group', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, but can you clarify how this makes adding the search boxes easier?
This PR is the first one for #1658
This PR only changes how selected options look like in both
Select
andMultiSelect
, to look like in the new UI designs:Everything related with
MultiSelect
checkboxes will come after.Changes
Before:
Grabacion.de.pantalla.desde.2024-08-19.11-56-42.webm
After:
Grabacion.de.pantalla.desde.2024-08-19.11-55-39.webm
Considerations
With these changes, selected options have a tick on the right side. This looks slightly awkward with some of the examples in the pattern library, specifically the ones with custom options.
Grabacion.de.pantalla.desde.2024-08-19.11-33-52.webm
However, customizing options is just a capability provided by the component, and not its own responsibility to ensure it looks good. Many other things could look awkward even with previous design, so I think this should be considered the consumer's responsibility.
This PR adds a second commit which adjusts those examples to look better.
Grabacion.de.pantalla.desde.2024-08-19.12-35-24.webm
Todo