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

Add Selector size variations and switch SelectorGroup to horizontal orientation #943

Merged
merged 5 commits into from
Jun 18, 2021

Conversation

mykolaharmash
Copy link
Contributor

@mykolaharmash mykolaharmash commented May 17, 2021

Addresses #931.

Purpose

Extending Selector and SelectorGroup features to support most common usecases.

Approach and changes

  • Add size prop to Selector
  • Refactor styling of the Selector to make sure its height matches other controls like Input and Select
  • Switch SelectorGroup to a horizontal orientation on a big screen and vertical on mobile
  • Add orientation prop to SelectorGroup to choose between horizontal and vertical layout.
  • Add size prop to SelectorGroup which changes size of a Selector within the group
  • Add gapSize prop to SelectorGroup to control the spacing between Selectors
  • Add stretch prop to SelectorGroup to make it fill all available width

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests Only updated snapshots as it's only styling changes, let me know if you think we need some new tests
  • Meets minimum browser support
  • Meets accessibility requirements

@changeset-bot
Copy link

changeset-bot bot commented May 17, 2021

🦋 Changeset detected

Latest commit: b9bf2f7

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

This PR includes changesets to release 1 package
Name Type
@sumup/circuit-ui Major

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

@vercel
Copy link

vercel bot commented May 17, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/DxXeL1HGQHwFSmTRoDBM7X7BaCq8
✅ Preview: https://oss-circuit-ui-git-selector-group-improvements-sumup.vercel.app

@sumup-clark
Copy link

sumup-clark bot commented May 17, 2021

Hey @mykolaharmash,
we are super excited that you are contributing! 🎉There's one more thing you need to do. Please accept our Contributor License Agreement. It helps you and us to collaborate on clear terms and focus on what we love most: code.

Thanks!

@codecov
Copy link

codecov bot commented May 17, 2021

Codecov Report

Merging #943 (b9bf2f7) into next (9548803) will increase coverage by 0.04%.
The diff coverage is 97.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #943      +/-   ##
==========================================
+ Coverage   91.67%   91.71%   +0.04%     
==========================================
  Files         165      165              
  Lines        3063     3091      +28     
  Branches      754      761       +7     
==========================================
+ Hits         2808     2835      +27     
- Misses        227      228       +1     
  Partials       28       28              
Impacted Files Coverage Δ
...cuit-ui/components/SelectorGroup/SelectorGroup.tsx 95.83% <92.85%> (-4.17%) ⬇️
...ckages/circuit-ui/components/Selector/Selector.tsx 100.00% <100.00%> (ø)

Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

Thanks for this great improvement to the Selector/SelectorGroup! 🙌

My main concern here is with versioning: as I mentioned in some of my comments, this looks like a major. If you need this earlier than Circuit V3, I think it could still be possible but we would need to avoid any breaking changes. I'd also love input from @connor-baer here 🙂

(Also, sorry I missed the issue at #931, we could have talked about the versioning questions earlier. Generally though, this is definitely the direction we're going into since there are design specs for it, so thanks for opening this PR! Let's figure out how to best release this.)

.changeset/hot-cheetahs-press.md Outdated Show resolved Hide resolved
packages/circuit-ui/components/Selector/Selector.docs.mdx Outdated Show resolved Hide resolved
packages/circuit-ui/components/Selector/Selector.tsx Outdated Show resolved Hide resolved
packages/circuit-ui/components/Selector/Selector.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

Thank you so much for addressing the changes! Just a few extra comments here, let's also go through them on our call and wrap this up today 🎉

packages/circuit-ui/components/Selector/Selector.tsx Outdated Show resolved Hide resolved
packages/circuit-ui/components/Selector/Selector.tsx Outdated Show resolved Hide resolved
.changeset/hot-cheetahs-press.md Outdated Show resolved Hide resolved
@mykolaharmash
Copy link
Contributor Author

mykolaharmash commented May 27, 2021

As agreed, we're going to make it a breaking change and release it with the next major version.

Latest changes:

  • Changing the base branch to next
  • Using box-shadow composition to render border and focus outline
  • Handling all the box-shadow styling in a dedicated function. This makes it easier to follow all the outline-related logic
  • Adding a state to track if Selector has focus. This also helps consolidate styling for the Label and get rid of input + label styles which were in a separate function before.

@robinmetral please have a look one more time.

Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

Just a few last things, it looks great 🎉 Thanks a lot for your work on this

packages/circuit-ui/components/Selector/Selector.docs.mdx Outdated Show resolved Hide resolved
packages/circuit-ui/components/Selector/Selector.tsx Outdated Show resolved Hide resolved
packages/circuit-ui/components/Selector/Selector.tsx Outdated Show resolved Hide resolved
.changeset/hot-cheetahs-press.md Outdated Show resolved Hide resolved
@mykolaharmash
Copy link
Contributor Author

Please give it another look, I've addressed the latest comments.

Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

Amazing! 🚀

You simply have outdated snapshots, the rest looks great. Thanks a lot for this!

@connor-baer
Copy link
Member

Thank you for the contribution @mykolaharmash and the thorough review @robinmetral. I'll take a quick look later today :)

Comment on lines +162 to +186
// +1px is to match the height of other form components
// like Input or Select that also have +1px for vertical padding
padding: `calc(${theme.spacings.byte} + 1px) ${theme.spacings.giga}`,
Copy link
Member

Choose a reason for hiding this comment

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

We will be able to remove this as part of DS-18.

packages/circuit-ui/components/Selector/Selector.tsx Outdated Show resolved Hide resolved
.changeset/hot-cheetahs-press.md Outdated Show resolved Hide resolved
@mykolaharmash
Copy link
Contributor Author

I've addressed the latest comments. Could you guys please merge current main into next? Because theme.borderRadius.tera is missing in next right now and it fails the build.

@connor-baer
Copy link
Member

@mykolaharmash The names for the border-radius values have been shifted, tera is now byte (see #980).

@mykolaharmash
Copy link
Contributor Author

mykolaharmash commented Jun 18, 2021

Fixed borderRadius. @connor-baer @robinmetral feel free to merge if everything looks fine 🙂

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.

Add size variations for <Selector> and horizontal orientation for <SelectorGroup>
3 participants