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

Latest Comments Block: Prefer relative units #24409

Conversation

aristath
Copy link
Member

@aristath aristath commented Aug 6, 2020

Changes the Latest Comments block to use relative instead of absolute units.
This will make theming easier, especially in cases where the theme changes the default font-size for the document. Sizes will now auto-scale, and themes won't need to rewrite styles, they can just enqueue the core styles.

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

Thanks for your work @aristath! - I believe in GB px are preferred as we can see from variables file for example:

$default-font-size: 13px;

Having said that, it's not my area of expertise so I added the design feedback label to be looked better.

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Aug 11, 2020

Perhaps for our own UI components it may make sense to use px sizes. But for front-end stuff like blocks, we should strive to match and integrate with the current theme. I've already seen too many 3rd party blocks (and before that, widgets and shortcodes) with styling that clashes with the rest of a website.

@ZebulanStanphill ZebulanStanphill added [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. CSS Styling Related to editor and front end styles, CSS-specific issues. [Block] Latest Comments Affects the Latest Comments Block labels Aug 11, 2020
@aristath
Copy link
Member Author

aristath commented Aug 12, 2020

Thanks Nick 👍

Most of the CSS-variables use pixels for the editor itself, not so much for the frontend styles. The remaining cases where pixels are used, are cases that need to be addressed. In multiple meetings with the themes team, the editor team, and members of the a11y team, the consensus always is that we want/need to start moving away from absolute units on all front-facing styles for blocks.
Moving to relative units benefits everyone and is less restrictive than px units as has been mentioned in multiple tickets.
This PR is one of many to convert block-styles from absolute to relative units, and simplifies theming a lot. Especially with FSE and global styles coming soon, relative units are even more important. When using relative units all blocks that use a relative unit will automatically be able to adjust their font-size and dimensions without needing to add more inline styles to override the defaults.

@jasmussen
Copy link
Contributor

All good thoughts, and I'd generally agree.

Many of the Gutenberg px values are for block UI (though a few are for the unstyled editing canvas). For anything that touches that, Nik is right. However aristath and Zeb are right when it comes to content of styles.scss for blocks, as this will be output on the frontend and ideally integrate seamlessly with the theme.

Principles and values here, are to provide the minimum CSS and styling in order to provide a nice block for a theme that doesn't style it, and otherwise be as adaptive as possible to the surroundings. I.e. try and inherit fonts and line heights, be careful with colors, all that stuff.

In that vein, high level, I do agree this is a good change.

I wonder though, whether rem is the right unit to use. It's widely supported (inc. IE11), but to me it also feels a little opinionated compared to using, say, em.

There's probably a long term solution here involving global styles, but as far as the near term solution, that seems like the one to agree on. @kjellr if you have time, do you have any insights? I'm happy to defer here as well.

@aristath
Copy link
Member Author

aristath commented Aug 12, 2020

Thanks @jasmussen!

Regarding em vs rem, it's a tricky topic...
Generally I prefer using em. The main issue with em however, is that it's cascading, while rem is relative to the root element. Ideally we'd use em values, and that would allow us for example to set the font-size on a group element and then have children scale accordingly. But... in previous attempts, using em messes up the editor 'cause it inherits the editor's styles and sizes too.

Using rem values is a reasonable middle-ground: Block sizes can scale, themes can change the root font-size in the :root or html element and have all blocks properly styled, and we avoid all the size-inheritance mess.

@jasmussen
Copy link
Contributor

jasmussen commented Aug 12, 2020

But... in previous attempts, using em messes up the editor 'cause it inherits the editor's styles and sizes too.

Oh, that's an important one to think about. While we mean for that to be a passing issue (we need to tackle at the root what casues CSS bleed) it is nevertheless real and present.

If you're okay waiting for our friend Kjell to wake up to the pings, I'd still appreciate his voice. However the above argument definitely ticked the weight towards rem over em for me.

@kjellr
Copy link
Contributor

kjellr commented Aug 12, 2020

Thanks for the ping. I need to reflect on this a little bit more, but here are some initial thoughts:

In general I agree that it makes sense to use rem instead of em here... but mostly just because rem will work as expected in WP-Admin, while em won't. 😄 The end goal would be to switch to em, since that's what I imagine most themes use on the front end. But that'll take a lot of extra work and I'm not sure it'll be possible anytime soon.

My main hesitation isn't around this PR, but more around the larger implications of doing this in all the other PRs... while switching to relative units is clearly the way this should be done, unfortunately it wasn't done from day one. That makes me a little concerned about how cumulative changes like this might end up having a noticeable difference on exsiting sites (in themes where 1rem ≠ 16px). Ideally, the change would mean that text and images are more in line with other theme styles, but even so it may be a little confusing for users.

Also on my mind is the fact that many themes (both Twenty Twenty and Twenty Nineteen for example) modify the value of 1rem on the front end, but not in the editor — it's impossible to specify a font-size value for the html selector if you're using add_editor_style. Even if you supplied a new 1rem value using the old enqueue_block_editor_assets hook, it may have unintended consequences if there happen to be any UI that uses rem values in WP. If we were to switch to rem values for blocks in the editor, I think we'd also want to a way for themes to safely set a value for 1rem.

@jasmussen
Copy link
Contributor

Great thoughts, Kjell, thanks so much.

Your explanation of the challenges with existing code and themes makes me think of this iframe PR: #21102 — if you read the conversation there, it becomes clear that it's both feasible, but also a giant change, and one that is likely to break a few things if/when it comes to pass. It's also a PR that incidentally should solve the CSS bleed problem, and make ems work both front and back.

I wonder if these changes should be bundled with such an iframe change, so that instead of lots of little annoying changes, there will be one big one, that happens to also come with a slew of very real and tangible benefits?

@aristath
Copy link
Member Author

so that instead of lots of little annoying changes, there will be one big one, that happens to also come with a slew of very real and tangible benefits?

I could easily do a single big PR instead of multiple small ones... Would be easier for me, but the reason I didn't do it is because we not only have to write the code, we also need to review it. Smaller incremental changes are easier to digest, review and merge 😅

@aristath aristath mentioned this pull request Aug 12, 2020
6 tasks
@kjellr
Copy link
Contributor

kjellr commented Aug 12, 2020

Would be easier for me, but the reason I didn't do it is because we not only have to write the code, we also need to review it. Smaller incremental changes are easier to digest, review and merge

It might be helpful even just to check out a draft PR that pulls in a lot of these changes — just so we can assess the overall impact.

For example, the more I think about this one...

Also on my mind is the fact that many themes (both Twenty Twenty and Twenty Nineteen for example) modify the value of 1rem on the front end, but not in the editor...

... the more I think that might be a problem. In this PR for instance, that would mean that for Twenty Twenty, all the changes in this PR would have a base rem value of 10px in the front end, but 16px in the editor. 😄 That could be a pretty noticeable change for folks.

@aristath
Copy link
Member Author

aristath commented Aug 12, 2020

It might be helpful even just to check out a draft PR that pulls in a lot of these changes — just so we can assess the overall impact.

Done, see #24523 👍

@aristath
Copy link
Member Author

Combined with other PRs in #24523

@aristath aristath closed this Aug 13, 2020
@aristath aristath deleted the aristath/latest-comments-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] Latest Comments Affects the Latest Comments 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. Needs Design Feedback Needs general design feedback. [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.

5 participants