Skip to content

Fix gradient color on onboarding for Spanish - #720

Merged
fredrikekelund merged 2 commits into
trunkfrom
update/onboarding-gradient-spanish
Dec 6, 2024
Merged

Fix gradient color on onboarding for Spanish#720
fredrikekelund merged 2 commits into
trunkfrom
update/onboarding-gradient-spanish

Conversation

@sejas

@sejas sejas commented Dec 4, 2024

Copy link
Copy Markdown
Member

Related issues

  • Fixes pdtkmj-3a1-p2#comment-5977

Proposed Changes

  • Fix gradient color for onboarding screen on Spanish

Testing Instructions

  • Start Studio and change the language to Spanish
  • Start Studio with no sites
  • Observe the gradient covers all the words
  • Try in different languages and observe the gradient looks good.
Before After
Screenshot 2024-12-04 at 15 08 38 Screenshot 2024-12-04 at 15 08 28

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?
@sejas sejas self-assigned this Dec 4, 2024
@sejas
sejas requested a review from a team December 4, 2024 15:12

@katinthehatsite katinthehatsite left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks good to me and the gradient works as expected:

Screenshot 2024-12-05 at 9 58 56 AM

I also tried in a couple more languages e.g. Russian and the changes look good there.

@fredrikekelund fredrikekelund left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change works, but it feels quite brittle to me. I played around with the text and found that setting line-height: normal on the L23 div moves the g descender inside the content box. It also increases the height of each row, so we need to compensate for that by setting a negative gap, margin or something.

I think it's worth the effort, though. This solution could easily break again down the line.

@sejas

sejas commented Dec 5, 2024

Copy link
Copy Markdown
Member Author

Thanks for the suggestion! I found that replacing leading-[42px] to leading-[48px] covers the word.
But in the end I've decided to just add a padding bottom and remove a gap-1, so the words stay at the same place.
Using the line-height (a.k.a leading- in tailwind) was better than my first approach, but I prefer this new solution (bae8de6) that doesn't use negative margings to force the positions.

Screenshot 2024-12-05 at 17 07 53

adding-padding-and-removing-gap-top

@fredrikekelund

Copy link
Copy Markdown
Contributor

The diff looks cleaner now, but we are still using an arbitrary value for the padding to make the gradient cover the entire g letter. For the record, the benefit of line-height: normal is that it adapts to the font—ensuring the entire letter stays within the content box.

Still, I acknowledge that it's unlikely this will come back and bite us anytime soon, so perhaps it's best to just land this PR and leave it this way for now. The solution definitely works 🙂

@fredrikekelund
fredrikekelund merged commit be2823c into trunk Dec 6, 2024
@fredrikekelund
fredrikekelund deleted the update/onboarding-gradient-spanish branch December 6, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants