[Components]: Add SegmentedControl #31937
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
force-pushed
the
add/segmented-control-component
branch
2 times, most recently
from
40b7691 to
51dd550
Compare
ntsekouras
force-pushed
the
add/segmented-control-component
branch
from
f4576d7 to
fd21789
Compare
|
Size Change: +3.48 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
ntsekouras
commented
Jun 17, 2021
ntsekouras
commented
Jun 17, 2021
|
Nice work. This is what I see:
There are some colors, metrics, sizes, tweaks, things to refine. I'll try and dive in and see if I can polish here. It also looks like an animation has been replaced with a fade for the indicator, instead of lateral movement like here:
I'll see if I can restore that. |
ntsekouras
commented
Jun 17, 2021
|
I pushed a few tweaks, now it looks like this:
We're getting closer here. One next step is fixing the focus style, which is supposed to be 1.5px admin-theme-color-blue. The CSS suggests this is accomplished by a border color and a .5px shadow:
But as you can see, the rule points to an object instead of a color. Here's the bit:
That "admin" color is defined here:
We need to revisit those admin color scheme treatments by the way, and ideally remove the fallback now that we don't have to support IE11. The colors are also wrong, where "medium blue" is not equivalent to the dark blue. @ntsekouras do you know why that bit isn't working as it should? |
I guess it's okay to change to have the proper value like Also I think it's okay to change to the proper color values in the --cc @sarayourfriend |
ntsekouras
commented
Jun 17, 2021
|
Note to self: if we remove the dark background color on the active button and rely on the backdrop, the horizontal sliding animation will be more visible. |
8 tasks
|
Fixed sliding:
We'll still need to remove the !important for the active control. Without it, it's essentially minimal specificity competing against minimal specificity:
In vanilla CSS I'd have done this: What's the CSS-in-JS equivalent? |
ntsekouras
commented
Jun 18, 2021
I refactored not to use While this works, the approach I took was to use |
packages/components/src/segmented-control/segmented-control-button.tsx
Outdated
Show resolved
Hide resolved
That's the exact right way to solve this problem, using the |
diegohaz
reviewed
Jun 23, 2021
ntsekouras
force-pushed
the
add/segmented-control-component
branch
from
b730335 to
d08bcbb
Compare
packages/components/src/segmented-control/segmented-control-button.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/segmented-control/segmented-control-option.tsx
Outdated
Show resolved
Hide resolved
diegohaz
reviewed
Jul 14, 2021
There was a problem hiding this comment.
This is so cool! Thanks so much @ntsekouras!
I'd like to discuss more the naming convention (SegmentedControl.Option vs. SegmentedControlOption), especially in the light of #33391. It doesn't look like we have many modules using this dot notation if any. So we would need to change more things if we want to adopt this convention. For now, I would stick with SegmentedControlOption.
My biggest concern on this Module.Child convention is that we can't leverage tree shaking. When Module is imported, all the Child components will be imported as well. For example, even if we just use Toolbar and Toolbar.Button, the other components of the family like Toolbar.DropdownMenu, Toolbar.Group etc. would also add to the bundle even though they're not being used.
diegohaz
reviewed
Jul 14, 2021
packages/components/src/segmented-control/segmented-control-option.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/segmented-control/segmented-control-option.tsx
Outdated
Show resolved
Hide resolved
diegohaz
reviewed
Jul 14, 2021
ntsekouras
commented
Jul 14, 2021
diegohaz
reviewed
Jul 14, 2021
packages/components/src/segmented-control/segmented-control-option.tsx
Outdated
Show resolved
Hide resolved
|
Oh, there's another thing regarding the focus ring style I think we should improve. To me, it feels too subtle, and it's hard to notice when the focus is changing:
(yes, it's a GIF and I'm pressing tab) What do you all think? I'm not sure how we're handling this in other components. |
I believe it's okay to land now as it's not been used somewhere yet. @jasmussen can take a look at it really soon, when he is back from his vacations. |
diegohaz
approved these changes
Jul 14, 2021
|
Thank you all for the help, reviews and guidance! 🙇 |
Oof this looks really bad. We should definitely address this. I'll add it to the list. |
|
Thanks for landing this! |
|
Congrats on landing this!
I'll take a look at this one as soon as I can! |
7 tasks
This was referenced Oct 12, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Adds a new
SegmentedControlcomponent.Testing Instructions
I have added a temp commit which renders this control in
PostFeaturedImage.Screen.Recording.2021-07-07.at.7.19.32.PM.mov