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

Try: apply the new type scale #18244

Closed
wants to merge 3 commits into from
Closed

Try: apply the new type scale #18244

wants to merge 3 commits into from

Conversation

davewhitley
Copy link
Contributor

@davewhitley davewhitley commented Nov 1, 2019

Description

This PR is an experiment to try and apply the new type scale, which is still a proposal at the moment.

Please note, I know there may be bugs, and there may be better ways to implement the type scale in code. This PR serves as a way to see the type scale in context. I did a few somewhat hacky things to override some styles.

Please give it a try and let me know what you think.

How has this been tested?

I have only visually tested the block editor.

Screenshots

Before/After gif https://make.wordpress.org/design/files/2019/10/comparison-editor.gif

Codepen of type scale https://codepen.io/drw158/pen/zbgEqO

After:

download-6

Types of changes

  • Created mixins for every type style
  • Updated various parts of Gutenberg to use the mixins
  • Minimal adjustments to spacing of some elements to solve visual defects

Visual changes:

  • This PR uses the new type scale, so the only visual changes are to type.
  • The default font size was increased to 14px, which is the biggest change.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@davewhitley
Copy link
Contributor Author

Sorry, I meant to add as a draft, and now I can't figure out how to convert it.

* Type scale mixins
*/

@mixin text-title-large {
Copy link
Contributor

@jameslnewell jameslnewell Nov 5, 2019

Choose a reason for hiding this comment

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

Should we prefix the mixins e.g. wp-text-title-large to avoid potential namespace clashes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems ok to me, since there will probably be editor specific font styles like editor-text-body.

@@ -1090,18 +1090,16 @@

.components-toolbar {
border: none;
line-height: 1;
font-family: $default-font;
Copy link
Contributor

@jameslnewell jameslnewell Nov 5, 2019

Choose a reason for hiding this comment

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

Should font-family be part of the mixin by default? The consumer can still override the font-family if they have a valid reason to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like we could get rid of font-family in the mixin.

$editor-font: "Noto Serif", serif;
$editor-html-font: Menlo, Consolas, monaco, monospace;
$editor-font-size: 16px;
$default-block-margin: 28px; // This value provides a consistent, contiguous spacing between blocks (it's 2x $block-padding).
$text-editor-font-size: 14px;
$editor-line-height: 1.8;
$big-font-size: 18px;
$big-font-size: 16px;
Copy link
Contributor

@jameslnewell jameslnewell Nov 5, 2019

Choose a reason for hiding this comment

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

Do these typography related variables still need to exist now that the mixins exist? Can they be removed or at least deprecated?

Copy link
Contributor Author

@davewhitley davewhitley Nov 13, 2019

Choose a reason for hiding this comment

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

The ones that are applied to the content of the post need to be kept separate and intact. The type scale shouldn't be applied in the content area because it represents the front end of the site, which is not in the scope of the type scale. For now, the type scale only applies to the application UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

🆒🆒

Why did it change as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think $big-font-size may be used for the UI, but I'd have to check and see why I changed it.

Copy link
Contributor Author

@davewhitley davewhitley Nov 13, 2019

Choose a reason for hiding this comment

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

Looks like it's not used that much, but I think I changed it because of this class

.block-editor-inserter__menu-help-panel-title {

which is some sort of title in the inserter popover.

Since it's only used a couple times, it might be easier just to eliminate the need for $big-font-size

@@ -37,3 +37,11 @@
@import "./components/warning/style.scss";
@import "./components/writing-flow/style.scss";
@import "./hooks/anchor.scss";

body {
@include text-body-small;
Copy link

Choose a reason for hiding this comment

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

@davewhitley Halloo!!! This PR looks great :).

The thing that stuck out to me was applying styles to the overall body. For a plugin-like thing like Gutenberg, it feels strange that it would target something so broad outside of itself.

For example, when this is loaded in WP-Admin, I'm not sure what body p may affect.

If we wanted to adjust "global" (in the context of Gutenberg) font styles, would we be able to target just Gutenberg?

Hope that makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we could specify Gutenberg only instead of body that'd be great (there's probably a good way to do that). I imagine it would take a few months to change those global styles in WP Admin.

Copy link

Choose a reason for hiding this comment

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

Just checked locally. It looks like the styles are being overridden anyway by the existing common.css 😅

Screen Shot 2019-11-14 at 08 37 19

Copy link

Choose a reason for hiding this comment

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

To apply things just to the editor, it looks like we have to update this file:
https://github.com/WordPress/gutenberg/blob/master/packages/edit-post/src/style.scss#L65

Adding the @include text-body-small mixin there appeared to work 👍

@ItsJonQ ItsJonQ self-requested a review November 13, 2019 09:40
@jameslnewell jameslnewell mentioned this pull request Nov 13, 2019
6 tasks
// Utility classes

.block-editor-page .text-caption {
@include text-caption;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -26,7 +29,7 @@
}

&__shortcut-term {
font-weight: 600;
// @include text-body-small;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to delete this.

@iamthomasbishop
Copy link

iamthomasbishop commented Dec 12, 2019

@davewhitley @jameslnewell I've been exploring how we might utilize this type scale in the native mobile version of Gutenberg, and overall I think it's going to work quite well. I have one main concern and that is letter-spacing. tl;dr my question is this: is it possible (or feasible, reasonable) to customize the letter-spacing based on which system font is used? The most-used ones (SF Pro and Roboto) have substantially different base character widths, and could thus benefit from fine-grain adjustments.

On native mobile GB, my plan was to follow the same type sizes, weights, etc. as y'all are defining here (we'd simply @import the type scale, afaik), but letter-spacing would essentially follow native values, separately for iOS and Android.

Here is my work-in-progress on mobile, closely following letter-spacing guidelines from Material Guidelines and HIG:

image

I understand setting everything to zero simplifies things a bit, but being an advocate for native I can't help but feel the urge to honor these type of details (pun fully intended). 🙃

EDIT: I just realized a couple of labels are off in the diagram above, but I think you get the point 😄

@jameslnewell
Copy link
Contributor

jameslnewell commented Dec 13, 2019

I've been exploring how we might utilize this type scale in the native mobile version of Gutenberg, and overall I think it's going to work quite well.

Awesome!

is it possible (or feasible, reasonable) to customize the letter-spacing based on which system font is used?

With the component added in #18495 it will be easy to customise the letter-spacing based on which platform (Web/iOS/Android) the component is being used on. I think iOS/Android usually uses a single hardcoded font unlike the font-stack on Web, so customising the letter-spacing on a per-font basis should be possible on those two platforms.

@iamthomasbishop
Copy link

@jameslnewell All good things to hear! Thanks for confirming 😄

@melchoyce
Copy link
Contributor

What can we do to move this PR forward?

@LevinMedia
Copy link

Also wanted to circle back and see if there is anything we can do to move this forward. Would love to be able to include this type scale in my designs.

@jameslnewell
Copy link
Contributor

jameslnewell commented Feb 16, 2020

#18495 (comment) has landed so you can start using the styles today.

import { __experimentalText as Text } from '@wordpress/components';

export const MyComponent = () => (
	<>
		<Text variant="title.large" as="h1">Title Large</Text>
		<Text variant="title.medium" as="h2">Title Medium</Text>
		<Text variant="title.small" as="h3">Title Small</Text>
		<Text variant="subtitle">Subtitle</Text>
		<Text variant="subtitle.small">Subtitle Small</Text>
		<Text variant="body">Body</Text>
		<Text variant="body.small">Body Small</Text>
		<Text variant="button">Button</Text>
		<Text variant="caption">Caption</Text>
		<Text variant="label">Label</Text>
	</>
);

There is still some work left to do in updating the existing components to use these styles.

@jameslnewell jameslnewell reopened this Feb 16, 2020
@jameslnewell
Copy link
Contributor

^^^ I accidentally mashed too many keyboard keys and closed the issue, but this PR probably can be closed now @davewhitley?

@jameslnewell
Copy link
Contributor

@iamthomasbishop @melchoyce @LevinMedia 🙏 I'd love some design input on the subtle changes/impact that applying the scale makes over here: #18652

Base automatically changed from master to trunk March 1, 2021 15:42
@ramonjd ramonjd added [Package] Base styles /packages/base-styles [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. labels Aug 26, 2021
@ramonjd
Copy link
Member

ramonjd commented Aug 26, 2021

This PR is a little stale and could probably be closed unless you think it's still relevant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Base styles /packages/base-styles [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants