Try: System font for vanilla editor styles. #26822
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasmussen
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
jasmussen
commented
Nov 9, 2020
packages/base-styles/_variables.scss
Outdated
| $editor-line-height: 1.8; | ||
| $big-font-size: 18px; |
There was a problem hiding this comment.
This variable has never been used.
jasmussen
commented
Nov 9, 2020
| @@ -9,7 +9,7 @@ | |||
| */ | |||
|
|
|||
| body { | |||
| font-family: $editor-font; | |||
| font-family: $default-font !important; // This !important is added temporary to start the conversation. It will need a core patch if we agree. | |||
There was a problem hiding this comment.
If we were to decide indeed that it was time to change the vanilla font, it would have to happen both here, and in WordPress. So it would need a separate PR, which I will make if this PR gets positive feedback.
|
If you would like to test this PR, you have to try it with a theme active which doesn't provide an editor style. The easiest, perhaps, is to search for |
aristath
reviewed
Nov 9, 2020
lib/edit-site-page.php
Outdated
| @@ -50,7 +50,7 @@ function gutenberg_get_editor_styles() { | |||
| ); | |||
|
|
|||
| /* translators: Use this to specify the CSS font family for the default font. */ | |||
| $locale_font_family = esc_html_x( 'Noto Serif', 'CSS Font Family for Editor Font', 'gutenberg' ); | |||
| $locale_font_family = esc_html_x( '-apple-system, BlinkMacSystemFont,"Segoe UI", Roboto, Oxygen-Sans, Ubuntu, Cantarell,"Helvetica Neue", sans-serif', 'CSS Font Family for Editor Font', 'gutenberg' ); | |||
There was a problem hiding this comment.
If we switch to system fonts this doesn't need to be translatable since they already work for all locales out of the box. I think we can safely remove the translation function and just add the CSS directly 👍
|
Size Change: +108 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
|
Overall I'd say this is a great improvement... It solves many issues at the same time.
|
|
I like it as an iteration. It makes sense and also benefits from being more performant. 👍🏻 |
|
Once this gets merged we can also close this one: https://core.trac.wordpress.org/ticket/46169 |
hc0503
approved these changes
Nov 10, 2020
|
It sounds like a nice win win PR. That will benefit all of us..:) |
|
This works well for me. Nice work!
|
|
Thank you Michael! I'll start to pair this up with a core patch, and maybe we can merge it in next week. |
jasmussen
force-pushed
the
try/vanilla-font-change
branch
from
c0a87b0 to
2de6ed6
Compare
|
Alright, I tweaked this PR to address the feedback — @aristath did I do it right? It also now no longer works if you just test the plugin. This has to be paired with https://core.trac.wordpress.org/ticket/46169 to take effect. Would appreciate input on both. |
|
Just wanted to say thank you all for the work here, looking forward to landing this change into core. |
jasmussen
force-pushed
the
try/vanilla-font-change
branch
from
2de6ed6 to
e19fb94
Compare
|
I made a change as proposed by @aristath to remove the editor font as a dependency. Could use a good sanity check that I did that right before this lands. Thank you. |
jasmussen
force-pushed
the
try/vanilla-font-change
branch
from
e26ccaa to
6b635c6
Compare
|
I'm having some test failures here that I'm not sure what to do with. Any input would be appreciated. |
|
The failing test doesn't seem to be related to the code in this PR... I merged the latest commits from master to this branch, let's see if that fixes the failing test 👍 EDIT: Yep, it fixed them 🥳 |
|
I could use some advice on whether to merge this one in. I know it's dependant on the trac counterpart, so I'd appreciate input on the timing. |
|
Hmmm WP5.6 comes out in a few days. In the meantime, WP-trunk is already for 5.7. With that in mind, I think the change in WP can be merged anytime (like... now). If the PR in Gutenberg gets merged now, then worst case scenario: Until WP5.7 comes out, users of the plugin that use a theme which does not define a font-family and falls-back to Noto will see system fonts instead. Which... IMO is fine. We may want to wait until WP5.7 goes to beta for example, but in any case the segment of users that will have an issue is pretty small, and worst case the browser will fallback to basically what this PR does (system fonts). |
|
Thanks. Since I don't think it's particularly critical we get this in 5.6, I'll wait and see if the 5.6 trac patch lands. If it does, we'll merge this right after. Otherwise, it'll be a 5.7 thing. Sound right? |
Yes, sounds good 👍 |
In the older days of the editor, few themes provided editor styles. In order to have contrast between block UI and editor UI, a Serif font was applied. It feels like we can revisit this: - Using system fonts allows has us load less stuff that is only ever used by themes that don't style the editor. - Because more themes style the editor, we can reposition the vanilla interface to be more of a writing interface, embrace that it isn't WYSIWYG. - The serif font hasn't aged well, and a sans-serif appears more legible in the context. This is not an urgent PR, and I'm sure there will be many opinions. Please do share yours!
jasmussen
force-pushed
the
try/vanilla-font-change
branch
from
dc68791 to
7e4a2fd
Compare
|
I've refreshed this one. It would be nice to get this one in 5.7. |
|
Ship it. |
dd32
added a commit
to dd32/gutenberg
that referenced
this pull request
Feb 12, 2021
aristath
pushed a commit
that referenced
this pull request
Feb 12, 2021
* Remove invalid quoted list of fonts. * Remove translator comment that's no longer needed after #26822. * Inline the font stack, as it's no longer translatable
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the older days of the editor, few themes provided editor styles. In order to have contrast between block UI and editor UI, a Serif font was applied. It feels like we can revisit this:
This is not an urgent PR, and I'm sure there will be many opinions. Please do share yours!
Before:
After: