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

[Components] Add SegmentedControl #405

Merged
merged 39 commits into from
Sep 28, 2021
Merged

[Components] Add SegmentedControl #405

merged 39 commits into from
Sep 28, 2021

Conversation

heyamykate
Copy link
Contributor

@heyamykate heyamykate commented Sep 28, 2020

Fixes #873

Figma

This PR adds a new component to the component library - <SegmentedControl />

It is very similar to the Tabs component we have - it accepts an array of objects to render into segments, and has an optional onClick handler prop as well. When a user clicks on one of the buttons, an "active state" button slides over to wherever the new active button resides in the DOM.

To run it in storybook format, pull down the branch and run yarn && yarn run storybook:dev and click on the SegmentedControl sidebar menu item.

Screen Shot 2020-09-29 at 3 21 31 PM

Screen Shot 2020-09-28 at 11 49 40 AM

@heyamykate heyamykate self-assigned this Sep 28, 2020
@snags88 snags88 temporarily deployed to curology-radiance-pr-405 September 28, 2020 18:35 Inactive
background-color: ${COLORS.purple10};
width: ${({ width }) => `${width}%`};
&:focus {
outline: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ben will probably be able to provide more guidance re: styling for different states, but there's a new focus variable you can use here to start playing around with how the component would work with only keyboard controls:

&:focus {
  outline: none;
  box-shadow: ${BOX_SHADOWS.focus};
}

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah, thanks for calling that out @michaeljaltamirano but we should apply the new focus styling to this: unsure on how it will react between the two options focus and focusInner but would it be possible to get a screenshot of both with the two scenarios below. These are how it looks with designs but unsure how it will with live code

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@heyamykate I ended up getting out of being underwater this cycle--can you re-tag me for review once the keyboard controls are in place?

Copy link
Contributor

Choose a reason for hiding this comment

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

@heyamykate It looks like there's some CSS finessing to match the above:

Active item has focus state:
Screen Shot 2020-10-07 at 10 20 55 AM

Inactive item has focus state:
Screen Shot 2020-10-07 at 10 20 48 AM

Copy link
Member

@LilCare LilCare Oct 20, 2020

Choose a reason for hiding this comment

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

@heyamykate The focused state looks perfect for the active item.
Screen Shot 2020-10-20 at 8 58 27 AM

However, if an inactive item adjacent still has a small piece cutoff:
Screen Shot 2020-10-20 at 8 58 35 AM

If you add a z-index, it should look like below:
Screen Shot 2020-10-20 at 9 01 36 AM

Copy link
Contributor

@benkolde benkolde left a comment

Choose a reason for hiding this comment

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

Looks good, once we figure out the focus piece it's good to go

@benkolde
Copy link
Contributor

Adding comment here for posterity:

The whole control should be 40px tall and should use the caption typography sizing

@heyamykate heyamykate marked this pull request as ready for review October 8, 2020 21:32
@heyamykate
Copy link
Contributor Author

@benkolde how is this for focus styling?

Here's the indicator/segment with focus styling applied
Screen Shot 2020-10-08 at 3 19 38 PM

Here's the inactive segment with focus styling
Screen Shot 2020-10-08 at 3 24 01 PM

@michaeljaltamirano
Copy link
Contributor

@heyamykate I ran out of time this week before going OOO--if you could get one more person to look at this it would be appreciated.

@snags88 snags88 temporarily deployed to curology-radiance-pr-405 December 10, 2020 13:58 Inactive
@michaeljaltamirano michaeljaltamirano changed the title Add new SegmentedControl component [Components] Add SegmentedControl Dec 22, 2020
@snags88 snags88 temporarily deployed to curology-radiance-pr-405 February 11, 2021 17:57 Inactive
Copy link
Contributor

@michaeljaltamirano michaeljaltamirano left a comment

Choose a reason for hiding this comment

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

I'm currently opening issues for UI toolkit / implementation discrepancies and our secondaryTheme was added after this PR was initially reviewed by folks so it looks like we will need to support the spec for the secondaryTheme version of this component now:

Screen Shot 2021-02-11 at 12 56 22 PM

@heyamykate
Copy link
Contributor Author

@michaeljaltamirano sorry I sort of forgot about this 😬 . I can work on this over cool-down 👍

@winstonkim
Copy link
Contributor

hi @michaeljaltamirano, I took a stab at making the styling updates for the secondary theme. let me know what you think!

also is there something specific i need to do so that the chromatic test passes?

@michaeljaltamirano
Copy link
Contributor

hi @michaeljaltamirano, I took a stab at making the styling updates for the secondary theme. let me know what you think!

also is there something specific i need to do so that the chromatic test passes?

The initial build will fail if there's new visual regression tests that haven't yet been vetted as the baseline, I went ahead and approved the changes and sent you an invite to join as a collaborator on Chromatic. I'll review this PR later--thanks for carrying the torch!

* Run ESLint fix

* Use BORDER_RADIUS.large (32px equivalent to 80px)

* Move border to items instead of container to account for absolutely positioned indicator

* Update snapshots

* Rename width to segmentWidth to not set width property to DOM element, remove horizontal padding

* Remove errant (redundant?) box-shadow, use padding instead of border so focus ring is consistent across control

* Add slight margin to get button dimensions better matching Figma file

* Add regression tests for focus states

* Update snapshot
Copy link
Contributor

@michaeljaltamirano michaeljaltamirano left a comment

Choose a reason for hiding this comment

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

@winstonkim Approving to unblock your work.

I noted a usability TODO in my Refinements PR, but it is not blocking for getting this merged in, releasing a new minor version, and unblocking your work. We can continue to refine it later.

Once the dust settles on what the DOM/UX looks/feels like, we should have it properly documented in Figma, akin to Ben's comment with some focus state designs, but official, based on whatever those future refinements end up being. We'll want to loop in design once we get a better sense of what is feasible.

However, as the TODO mentions, it might also be worth working with Design to determine whether or not this control is better represented as separate components: a TabList which would seem to operate like how we seem to be needing it for some new Site pages being worked on (assuming I looked at the correct LP), and/or a RadioGroup if the selections correspond to some form submissions. If they're purely presentational then I think a TabList may be preferable to our current button implementation.

@winstonkim winstonkim merged commit 40b7201 into master Sep 28, 2021
@winstonkim winstonkim deleted the add-new-tab-component branch September 28, 2021 15:31
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.

Add Segmented Control
6 participants