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

SegmentedControl #2083

Merged
merged 44 commits into from
Jul 22, 2022
Merged

SegmentedControl #2083

merged 44 commits into from
Jul 22, 2022

Conversation

simurai
Copy link
Contributor

@simurai simurai commented May 19, 2022

What are you trying to accomplish?

This adds the SegmentedControl component.

Screen Shot 2022-07-21 at 11 39 05

Screen Shot 2022-07-21 at 11 39 19

👀 Storybook preview.

TODO

What approach did you choose and why?

Implementation is based on (some are internal links):

What should reviewers focus on?

I left a few comments with some open questions.

Can these changes ship as is?

  • Yes, this PR does not depend on additional changes. 🚢

@changeset-bot
Copy link

changeset-bot bot commented May 19, 2022

🦋 Changeset detected

Latest commit: 9b2bf73

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@simurai simurai temporarily deployed to github-pages May 19, 2022 08:47 Inactive
@simurai simurai temporarily deployed to github-pages May 26, 2022 08:59 Inactive
@simurai simurai temporarily deployed to github-pages May 31, 2022 12:00 Inactive
@simurai simurai temporarily deployed to github-pages June 1, 2022 15:28 Inactive
@simurai simurai temporarily deployed to github-pages June 1, 2022 15:36 Inactive
Comment on lines 46 to 47
// TODO: Change to aria-current or aria-selected?
&[aria-pressed='true'] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the comment says: Is aria-pressed correct in this case? It's based on this Figma example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/segmented-control/segmented-control.scss Show resolved Hide resolved
src/segmented-control/segmented-control.scss Outdated Show resolved Hide resolved
src/segmented-control/segmented-control.scss Outdated Show resolved Hide resolved
@simurai simurai temporarily deployed to github-pages June 1, 2022 19:51 Inactive
@simurai simurai temporarily deployed to github-pages June 10, 2022 20:00 Inactive
@mperrotti
Copy link
Contributor

Thanks for the explanation @simurai

I think I understand. I'll add these styles to the React and Figma implementations

mperrotti
mperrotti previously approved these changes Jun 21, 2022
@mperrotti mperrotti dismissed their stale review June 21, 2022 21:36

I just realized the hover/active background color isn't inset like it is in Primer React

@github-actions github-actions bot temporarily deployed to Storybook Preview July 20, 2022 09:02 Inactive
@simurai simurai temporarily deployed to github-pages July 20, 2022 09:05 Inactive
@github-actions github-actions bot temporarily deployed to Storybook Preview July 20, 2022 09:05 Inactive
@simurai simurai temporarily deployed to github-pages July 21, 2022 02:20 Inactive
@github-actions github-actions bot temporarily deployed to Storybook Preview July 21, 2022 02:20 Inactive
@simurai
Copy link
Contributor Author

simurai commented Jul 21, 2022

@mperrotti I just realized the hover/active background color isn't inset like it is in Primer React

This is now updated to align with PRC. See Storybook

@simurai simurai requested a review from mperrotti July 21, 2022 02:38
@simurai simurai enabled auto-merge (squash) July 21, 2022 02:41
@simurai
Copy link
Contributor Author

simurai commented Jul 22, 2022

Ok, I'm gonna merge this "as is". Once we start with the PVC implementation, we can come back and make follow-up PRs if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants