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

Set base typographic rhythm #2369

Closed
wants to merge 4 commits into from

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Jun 10, 2020

Fixes #550

Adds support in ReactAztecText (Android only for now) for the lineHeight property. At the same time, setting the default fontSize to 16(px) and default lineHeight to 26(px).

Note: Since this PR changes the defaults for the RichText component, blocks that depend on it might change as well. For example the List block, Heading block and others.

Gutenberg PR: WordPress/gutenberg#23079

WPAndroid PR for testing this with a prebuilt APK: wordpress-mobile/WordPress-Android#12156

Keeping this PR as a draft until we decide whether to wait for the iOS side implementation to happen on this PR, or not.

To test:

  1. Run the demo app and have a paragraph block with a handful of lines of text
  2. Check that the line spacing is around 1.6x

Screenshots

Before After
Screenshot_1591827487 Screenshot_1591827786
Screenshot_1591828274 Screenshot_1591827994

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@hypest hypest added the [Status] Needs Design Review Needs design review or sign-off before shipping label Jun 10, 2020
@hypest hypest requested a review from iamthomasbishop June 10, 2020 22:21
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 10, 2020

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@etoledom
Copy link
Contributor

I'm implementing setting line height on iOS, it works but I haven't pushed since I'm seeing a few problems with it.

In general terms, it's good to let the system take care of line spacing, and set it manually just as exceptions when it's required.

In this case, having a font size that is smaller than the manually set line height is giving us some troubles on List Block and Preformated block:

Before After
IMG_0104 IMG_0103

The text inside a bigger line will fall to the bottom, having extra space at the top that is visible at the top of the preformatted block, and makes the bullets unaligned on list blocks.

cc @iamthomasbishop @SergioEstevao

@hypest
Copy link
Contributor Author

hypest commented Sep 18, 2020

Thank you @etoledom !

Closing this PR until we go back working on #550 as it has gone stale.

@hypest hypest closed this Sep 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Design Review Needs design review or sign-off before shipping
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve base typographic rhythm
2 participants