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

Paragraph block: Use relative instead of absolute units #24330

Closed
wants to merge 1 commit into from

Conversation

aristath
Copy link
Member

@aristath aristath commented Aug 3, 2020

This PR converts the paragraph block to use rem values instead of px.
This will provide better compatibility with themes that use a different base font-size, as lately we're seeing a tendency to use a base font-size of 20px+.
With these changes in place, themes won't need to rewrite all CSS and can just use what's provided by core.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation. (Not applicable)
  • I've included developer documentation if appropriate. (Not applicable)
  • I've updated all React Native files affected by any refactorings/renamings in this PR. (Not applicable)

Copy link
Member

@ZebulanStanphill ZebulanStanphill left a comment

Choose a reason for hiding this comment

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

I checked, the math all lines up, so the relative sizing of each font size preset remains the same.

Aside from one questionable change to the background padding, I think this is a good idea. But I think it would be good to get some more opinions on this.

Comment on lines 31 to 33
p.has-background {
padding: $block-bg-padding--v $block-bg-padding--h;
padding: 1.25em 2.375em;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is the only change I'm not sure of. If we want to change this, shoudn't we change it at the variable itself? Should we even change it in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this part was also troubling me a bit... so I pushed it as a point for discussion.
If users add a small-text, should it have smaller padding than a large-text? If yes, then em values make sense. If on the other hand we want consistent paddings that don't depend on the font-size, then em doesn't make sense and could be changed to rem values.
The reason I didn't change the variables themselves is because they are used in other blocks as well (columns & heading) and the change would affect more styles than intended. 🤔

If the padding change is a blocker for this PR I can just remove it and bring it up in a meeting for further discussion.

@ZebulanStanphill ZebulanStanphill added [Block] Paragraph Affects the Paragraph Block [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Enhancement A suggestion for improvement. CSS Styling Related to editor and front end styles, CSS-specific issues. labels Aug 3, 2020
@ZebulanStanphill
Copy link
Member

End-to-end test failure is intermittent and unrelated. See #24336.

@aristath
Copy link
Member Author

aristath commented Aug 4, 2020

Also related to #24323 & #24322

@aristath
Copy link
Member Author

Combined with other PRs in #24523

@aristath aristath closed this Aug 13, 2020
@aristath aristath deleted the p-block-relative-units branch August 13, 2020 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Paragraph Affects the Paragraph Block CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Package] Block library /packages/block-library [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants