Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SegmentedControl #2083
SegmentedControl #2083
Changes from 35 commits
48b39d5
c25185f
af466be
ec54b85
d2e7738
9d052fb
f99cab2
746a284
20f4ca5
539e249
2828547
d387489
856d2af
065151c
4089e1a
f2057fa
5a522c9
f9e5a20
2b6206a
6db93a2
829a386
c004088
49c6357
46e545b
3831e4b
4ee0844
abf3153
a90d72b
2994f39
3d98ee8
92e53f1
912039f
fb6807f
2af922d
5d1517e
660aa5b
c7077df
e3baab2
a39eaaf
be4188c
70b7019
147c07c
b426825
9b2bf73
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Just a heads up: I'm experimenting with an inset hover background in my Primer React PR (Storybook preview deployment). It's not quite right yet, but let me know what you think.
Demo:
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.
Interesting.. it feels a bit too much inset. But yeah, we would not need to hide the dividers when hovering, which makes it simpler.
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 agree that it's too inset. I'm iterating on it.
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 briefly explored that hover state– wondering if it might make sense to go lighter, as to imply it will be selected (the lightest shade) on click
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 think that's a good idea for this component, but we usually go darker on
:hover
and:active
(at least for light mode).If we went lighter, we'd probably need component-specific color tokens. If we go darker, we could share a color token between other interactive elements like ActionList items.
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 think in our sync with @vdepizzol we decided light didn't make sense here 😄 so disregard my suggestion! I agree Mike we should try and stick with standard conventions.
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.
Thanks @langermank - I completely forgot about the discussion in our sync 😅
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.
Ok, it now uses a
3px
inset padding: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.
@mperrotti Primer React implementation should align with this design decision.