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

Update default component heights #984

Merged
merged 13 commits into from
Jun 22, 2021
Merged

Update default component heights #984

merged 13 commits into from
Jun 22, 2021

Conversation

amelako
Copy link
Contributor

@amelako amelako commented Jun 18, 2021

Addresses #859.

Purpose

A consistent height for components has the following benefits:

Seamless experience across platforms (web views)
Optimised touch size for mobile web
Modular approach / different components composable
Simplification of component size definition

Approach and changes

  • Changed the button component default size from mega to giga (48px).
  • Added size prop for the Spinner component with size values byte, kilo (default) and giga.
  • Added new Icon size tera in design-tokens.
  • Changed the Input, Search and Select components height from size mega to giga (48px)
  • Changed Tabs height from 80px to size giga(48px).
  • Changed Tag height from 34px to 32px.

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@sumup-clark
Copy link

sumup-clark bot commented Jun 18, 2021

Hey @amelako,
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!

@changeset-bot
Copy link

changeset-bot bot commented Jun 18, 2021

🦋 Changeset detected

Latest commit: e9e78a7

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

This PR includes changesets to release 2 packages
Name Type
@sumup/circuit-ui Major
@sumup/design-tokens 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 Jun 18, 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/4XumaeFfGCD3PqXN4bjukkMpNuVT
✅ Preview: https://oss-circuit-ui-git-update-component-heights-sumup.vercel.app

@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #984 (e9e78a7) into next (61c15cf) will increase coverage by 0.02%.
The diff coverage is 96.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #984      +/-   ##
==========================================
+ Coverage   91.68%   91.70%   +0.02%     
==========================================
  Files         164      165       +1     
  Lines        3091     3112      +21     
  Branches      758      758              
==========================================
+ Hits         2834     2854      +20     
- Misses        227      228       +1     
  Partials       30       30              
Impacted Files Coverage Δ
...ages/circuit-ui/components/DateInput/DateInput.tsx 94.11% <ø> (ø)
...cuit-ui/components/LoadingButton/LoadingButton.tsx 95.23% <ø> (ø)
.../circuit-ui/components/SearchInput/SearchInput.tsx 100.00% <ø> (ø)
packages/circuit-ui/components/Select/Select.tsx 98.52% <ø> (ø)
packages/circuit-ui/components/Tag/Tag.tsx 97.56% <ø> (ø)
packages/design-tokens/themes/shared.ts 0.00% <ø> (ø)
packages/design-tokens/utils/theme-prop-type.ts 0.00% <ø> (ø)
...ages/circuit-ui/cli/migrate/button-default-size.ts 94.73% <94.73%> (ø)
packages/circuit-ui/components/Button/Button.tsx 98.07% <100.00%> (ø)
...es/circuit-ui/components/IconButton/IconButton.tsx 100.00% <100.00%> (ø)
... and 4 more

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.

Some extra things left:

  • Changesets. I would suggest:
    • One general one for adapting the UI (major)
    • One for the new Spinner size prop (major)
    • One for the Button API breaking change (major)
    • One for the new iconSize value (design-tokens, minor)
  • Codemods
    • Button: the size changes (mega becomes giga)
    • Spinner: we should add size="byte" to all <Spinner /> to keep them the same size as they used to be

@amelako amelako marked this pull request as ready for review June 18, 2021 14:40
Copy link
Member

@connor-baer connor-baer left a comment

Choose a reason for hiding this comment

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

When writing documentation and changesets, I recommend following these general guidelines for concise, easy-to-understand text:

  1. Use simple language, short sentences, and precise words.
  2. Provide required context. Each section should be understandable in isolation.
  3. (For changesets specifically) Use the imperative mood in the past tense.

You can find more helpful writing guidelines here.

Copy link
Member

@connor-baer connor-baer left a comment

Choose a reason for hiding this comment

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

Well done 👏🏻👏🏻👏🏻

@robinmetral
Copy link
Contributor

Note: we won't add a codemod for the Spinner size change (the only size previously was byte, the new default is kilo) because we generally will want kilo sizes. In specific cases where byte might be desired, the size attribute can be manually added.

@amelako amelako merged commit 7879a99 into next Jun 22, 2021
@amelako amelako deleted the update-component-heights branch June 22, 2021 09:02
@github-actions github-actions bot mentioned this pull request Jun 22, 2021
@connor-baer connor-baer linked an issue Jun 29, 2021 that may be closed by this pull request
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.

Update default component heights
3 participants