GetJobber / atlantis

🔱 Atlantis
https://atlantis.getjobber.com
MIT License
25 stars 30 forks source link

feat(components): Add description prop to InputFile #2019

Closed taylorvnoj closed 1 week ago

taylorvnoj commented 2 weeks ago

Motivations

The purpose behind adding supportText is to communicate to a user, the permitted file types and max file size when uploading files via InputFile dropzone. This can help in avoiding upload errors that need to be rectified after a failed upload.

Changes

Added

  1. supportText prop, which is a string. Why a string?
    • Although the motivation behind adding this prop is to display file types and size limit for files, I feel that keeping this as customizable as possible, allows for flexibility moving forward
    • Allowing for a string also makes localization easier on the consumer side

Screenshot 2024-09-11 at 5 53 46 PM

Figma

Why not dynamically use allowedTypes here?
Thinking about instances where consumers are putting allowedTypes into an array, if they allow DOCX files, that looks like this: "application/vnd.openxmlformats-officedocument.wordprocessingml.document". I personally do not think it's worth having all possible MIME types (there's too many) mapped in this component, just to convert them to uppercase and then a string to be displayed with a file size that is set in the consumers component. This would not only be adding a lot of logic to this component, but I can foresee that becoming brittle.

  1. Added an example of supportText to the Variations and Sizes story, as well as a stand alone Support Text story

Changed

The hintText is no longer combined with the Button text. That sentence format spanning across button and hintText will not scale well with various languages in the future.

Testing

Changes can be tested via Pre-release


In Atlantis we use Github's built in pull request reviews.

Random photo of Atlantis

cloudflare-workers-and-pages[bot] commented 2 weeks ago

Deploying atlantis with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1d39876
Status: âœ…  Deploy successful!
Preview URL: https://dd6848d7.atlantis.pages.dev
Branch Preview URL: https://job-96785-job-101450-add-sup.atlantis.pages.dev

View logs

github-actions[bot] commented 1 week ago

Could not publish Pre-release for c542c3ec9ef83fea17cc6e593a964b2cf8ab58a2. View Logs The problem is likely in the NPM Publish or NPM CI step in the Trigger Pre-release Build Job.

Previous build information:

Published Pre-release for 651a43ea58ef9a09585caec1042c9bdc924f7770 with versions:

  - @jobber/components@5.29.2-JOB-96785-651a43e.2+651a43ea

To install the new version(s) for Web run:


npm install @jobber/components@5.29.2-JOB-96785-651a43e.2+651a43ea
```.
chris-at-jobber commented 1 week ago

@taylorvnoj did you look at similar properties on other components when naming this prop? The two patterns I see that are similar are

description or assistiveText

image image

As you can see, typically the components-native versions use assistiveText while the web versions use description, which is an unfortunate discrepancy, but it seems like this prop does something similar and should follow one of those approaches - although if you are referencing another pattern or approach to this, happy to discuss!

taylorvnoj commented 1 week ago

@chris-at-jobber great call out - admittedly I was narrow focused on this component and wasn't doing the full homework. SHAME! Renamed to description here 7790894e536bd530bf299263c5b6ead2ac031a6d