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

Fix about screen vertical spacing #5576

Merged
merged 2 commits into from Dec 31, 2021
Merged

Conversation

malinajirka
Copy link
Contributor

@malinajirka malinajirka commented Dec 30, 2021

Description

This PR replaces dp with sp for line height. Technically, even dp are supported to be used for lineHeight. However, there is a bug in Compose Theme Adapter which breaks vertical spacing when dp is used. It's quite tricky since lineHeight in Compose accepts only TextUnit which doesn't support dp.

I'm on the edge if we should update our styles from dp to sp or wait for the fix in the library. However, I'm inclined towards updating our code as I suspect this might cause even other side-effects/bugs and we might waste time on them in the future. The change has very minimal impact on the UI only when the OS font size settings is not set to default.

Props to @AnirudhBhat for noticing the issue. It occurs only in jalapeno builds since vanilla and wasabi have shorter app name so the app name fits one line.

P.S. This PR also updates the hashcode of the Automattic About library. This doesn't have any affect, I just forgot to update the commit hash with the merge commit in the previous PR.

Testing instructions

  1. Use jalapeno build
  2. Open App settings
  3. Tap on "WooCommerce (Pre-Alpha)" row under "About the app" section
  4. Notice the screen looks ok

Change font size in OS settings and smoke test the app -> there will be some changes in lineHeight spacing, but they should be minimal. Notice for example the Top Performers section on the screenshots below.

Images/gif

Before After
Screenshot_1640867963 Screenshot_1640868176
Screenshot_1640862208 Screenshot_1640862197
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@malinajirka malinajirka added the category: design Layout and style elements in the UI or user interface, including color and animations. label Dec 30, 2021
@malinajirka malinajirka added this to the 8.3 milestone Dec 30, 2021
@malinajirka malinajirka added this to In Review in Continuous Improvements via automation Dec 30, 2021
@peril-woocommerce
Copy link

You can test the changes on this Pull Request by downloading the APK here.

@AnirudhBhat AnirudhBhat self-assigned this Dec 31, 2021
@AnirudhBhat
Copy link
Contributor

@malinajirka Excellent job researching the bug and digging deep into the root cause 🙇 I've tested the app and it looks good to me 🚢

@AnirudhBhat AnirudhBhat merged commit bbd2707 into trunk Dec 31, 2021
@AnirudhBhat AnirudhBhat deleted the fix-about-screen-vertical-spacing branch December 31, 2021 06:08
Continuous Improvements automation moved this from In Review to Done Dec 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: design Layout and style elements in the UI or user interface, including color and animations.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants