Size prop added to ImageInput component

[giedoka]

Purpose

ImageInput has 2 sizes. They have different button size. In that case, ImageInput component needs to have buttonSize prop.

Approach and changes

• border radius docs updated
• fix of border radius in Avatar (there was wrong value for the giga one)
• buttonSize prop added to ImageInput

Definition of done

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

[changeset-bot]

🦋 Changeset detected

Latest commit: 1f5718a

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	Minor

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]

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/GTChasDruHg1LMx9eeSSyezRfTif
✅ Preview: https://oss-circuit-ui-git-feature-image-input-size-sumup.vercel.app

[sumup-clark]

Hey @giedoka,
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]

Codecov Report

Merging #1260 (1f5718a) into main (96d0007) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1260      +/-   ##
==========================================
+ Coverage   92.03%   92.05%   +0.01%     
==========================================
  Files         184      184              
  Lines        3630     3639       +9     
  Branches     1170     1171       +1     
==========================================
+ Hits         3341     3350       +9     
  Misses        271      271              
  Partials       18       18              

Impacted Files	Coverage Δ
packages/circuit-ui/components/Avatar/Avatar.tsx	95.23% <100.00%> (+1.12%)	⬆️
...es/circuit-ui/components/ImageInput/ImageInput.tsx	86.45% <100.00%> (+0.74%)	⬆️

[robinmetral]

Thanks for the PR @giedoka! Added a few comments, mostly the last one is an important discussion we should have about the API for this change 🙂

[robinmetral]

The size here should be optional, otherwise it would be a breaking change

[robinmetral]

Actually I just got back into the stories and I realized something: we probably don't need this extra size prop at all! Back then, we built the component that way to prevent tying the ImageInput together with the Avatar too much—basically you could use the ImageInput with any visual component that accepts an src and alt text (although it's optimized for the Avatar).

So this means that we can get the small ImageInput like this in userland:

// this is an example from the stories
<ImageInput
  label="Upload an image"
  clearButtonLabel="Clear"
  src={imageUrl}
  onChange={onChange}
  onClear={onClear}
  invalid={!!error}
  validationHint={error}
  loadingLabel="Uploading"
  // component={Avatar} 👈 instead of just passing the Avatar...
  component={({ src, alt }) => <Avatar src={src} alt={alt} size="giga" />} // 👈 ...we pass it with the `size` prop
/>

So instead of forwarding the size to the underlying component, I think what we could do here is accept a new, optional buttonSize prop, which could be either giga or yotta (or maybe it should be more aligned with the IconButton component and be either kilo (the existing small IconButton) or byte (the new XS IconButton—name to confirm with GC).

That way the ImageInput stays decoupled from the Avatar, and developers choose which button size they want for their own use-case.

We would also add a new story for the ImageInput using an <Avatar size="giga" /> since that might be a fairly common use case.

cc @connor-baer, would love your thoughts here. Let me know if you need background on anything in this PR

[connor-baer]

I agree that the ImageInput should remain independent of any particular image component. If multiple sizes of the ImageInput are needed, I would therefore keep the size of the ImageInput independent of the child component as well. In other words, we shouldn't pass the size to the child component.

I would keep the name of the size prop for the ImageInput generic. Right now, it only affects the button, but what if additional elements are added in the future that also need to adapt their size?

[robinmetral]

I would keep the name of the size prop for the ImageInput generic. Right now, it only affects the button, but what if additional elements are added in the future that also need to adapt their size?

@giedoka we also forgot about this! That was a good point—the ImageInput's only built-in UI is the button, which means we can just rename buttonSize into size.

We could use the prop description to say that this changes the button size, so make it more intuitive for developers without requiring breaking changes in the future if the size affects anything else 🙂

[giedoka]

Ahhh yes good point! I focused on adding new prop but not on its name... I'll change it

[robinmetral]

Looks great! Final comments here (mostly nitpicks), thanks a lot for your patience 🙏

[robinmetral]

Yay! My last comment is a tiny wording thing for the size prop description so I'm approving already 😅

I'll request a review from design on Chromatic in the meantime.

Thanks you so much for contributing this! 🎉