Doist / reactist

Open source React components made with ā¤ļø by Doist
http://doist.github.io/reactist
MIT License
250 stars 22 forks source link

fix: Change Badge component to bold + set explicit line height #755

Closed engfragui closed 1 year ago

engfragui commented 1 year ago

Short description

This PR tweaks our (existing) Badge component:

  1. Content of the Badge is now bold (as per this request on Twist). We should be able to see this very clearly via our visual regression tests, but here's some screenshots:

    BeforeAfter
    Screenshot 2023-02-17 at 12 57 43 Screenshot 2023-02-17 at 12 25 37
  2. The appropriate line-height is enforced. No line-height set was causing the background of the badge to expand vertically when inside flex containers:

    Screenshot 2023-02-17 at 12 51 43

    Although this can be easily set in the client, I figured we would want to enforce this in Reactist once and for all.

I'm also taking this chance to fix a couple of typos in the Banner stories (25 instead of 24 px -- I was probably drunk, who knows šŸ¤·ā€ā™€ļø).

PR Checklist

Versioning

I think this is a minor fix, so I'm just bumping the patch.

engfragui commented 1 year ago

We should be able to see this very clearly via our visual regression tests

Errr not true (see this build -- you can see that there's no side by side comparison of all the badges). Looking into it now šŸ‘€

Edit: I think the

{PlaygroundTemplate.bind({})}

inside the main storybook demo prevents the tests from working correctly šŸ¤”

gnapse commented 1 year ago

@engfragui upon inspecting the Chromatic snapshots:

CleanShot 2023-02-17 at 10 03 46@2x

I find it odd that in there the vertical padding looks almost non-existent. Could it be that 1px is too little?

engfragui commented 1 year ago

Could it be that 1px is too little?

@gnapse Errr the Figma mocks say the padding should be 3px šŸ˜…

Screenshot 2023-02-17 at 14 06 16

I think it still looked good because the background was anyway expanding vertically to fill the entire vertical height, so we never noticed this. I will change to 3px.