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

Improve type for icon prop #1615

Merged
merged 4 commits into from
May 22, 2023
Merged

Improve type for icon prop #1615

merged 4 commits into from
May 22, 2023

Conversation

connor-baer
Copy link
Member

@connor-baer connor-baer commented Jun 13, 2022

Purpose

While working on the new https://developer.sumup.com, I found a couple of inconsistencies in Circuit UI. I tried to narrow the type of the icon prop in all components that accept it by specifying the accepted sizes. However, TypeScript throws a fit whenever the size prop doesn't match exactly.

Instead, I changed the default size to any to work around this TypeScript limitation and added a new IconComponentType export to the @sumup/icons package.

Approach and changes

  • Changed the IconProps default size type to any
  • Automatically set the size prop on the Button's icon prop based on the Button's size prop
  • Properly hide icons inside a Button

Definition of done

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

@changeset-bot
Copy link

changeset-bot bot commented Jun 13, 2022

🦋 Changeset detected

Latest commit: 6727601

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/icons 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 13, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oss-circuit-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 22, 2023 8:30am

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #1615 (6727601) into next (f53e433) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1615      +/-   ##
==========================================
- Coverage   96.88%   96.87%   -0.01%     
==========================================
  Files         258      258              
  Lines       23186    23188       +2     
  Branches     2160     2164       +4     
==========================================
+ Hits        22463    22464       +1     
  Misses        715      715              
- Partials        8        9       +1     
Impacted Files Coverage Δ
packages/circuit-ui/components/Button/Button.tsx 90.87% <100.00%> (+0.09%) ⬆️
...es/circuit-ui/components/IconButton/IconButton.tsx 94.31% <100.00%> (ø)
...ckages/circuit-ui/components/ListItem/ListItem.tsx 94.27% <100.00%> (-0.49%) ⬇️
...es/circuit-ui/components/Notification/constants.ts 100.00% <100.00%> (ø)
packages/circuit-ui/components/Popover/Popover.tsx 99.77% <100.00%> (+0.22%) ⬆️
...avigation/components/UtilityLinks/UtilityLinks.tsx 100.00% <100.00%> (ø)

@connor-baer connor-baer mentioned this pull request Jun 13, 2022
5 tasks
packages/circuit-ui/package.json Outdated Show resolved Hide resolved
@github-actions

This comment was marked as outdated.

@connor-baer connor-baer marked this pull request as ready for review May 22, 2023 08:30
@connor-baer connor-baer requested a review from a team as a code owner May 22, 2023 08:30
@connor-baer connor-baer requested review from pdrmdrs and removed request for a team May 22, 2023 08:30
@connor-baer connor-baer added the 🚢 ready to merge Automatically merge the PR once all requirements are met label May 22, 2023
@kodiakhq kodiakhq bot merged commit 51cd70d into next May 22, 2023
@kodiakhq kodiakhq bot deleted the bugfix/icon-prop branch May 22, 2023 11:27
This was referenced Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗂 circuit-ui 🗂 icons 🚢 ready to merge Automatically merge the PR once all requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants