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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7219d6
Add mixins for type styles
davewhitley Nov 1, 2019
73f4e3d
Adjust default font size variables to adhere to type scale
davewhitley Nov 1, 2019
d59e72a
Apply type styles to block editor
davewhitley Nov 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions packages/base-styles/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,83 @@
/**
* Type scale mixins
*/

@mixin text-title-large {
Copy link
Contributor

@jameslnewell jameslnewell Nov 5, 2019 •
edited
Loading

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.

$font-size: 32px;
font-weight: 400;
font-size: $font-size;
line-height: (40px / $font-size); // line-height in px divided by font-size
}

@mixin text-title-medium {
$font-size: 24px;
font-weight: 400;
font-size: $font-size;
line-height: (32px / $font-size); // line-height in px divided by font-size
}

@mixin text-title-small {
$font-size: 20px;
font-weight: 400;
font-size: $font-size;
line-height: (28px / $font-size); // line-height in px divided by font-size
}

@mixin text-subtitle {
$font-size: 16px;
font-weight: 600;
font-size: $font-size;
line-height: (24px / $font-size); // line-height in px divided by font-size
}

@mixin text-subtitle-small {
$font-size: 14px;
font-weight: 600;
font-size: $font-size;
line-height: (20px / $font-size); // line-height in px divided by font-size
}

@mixin text-body {
$font-size: 16px;
font-weight: 400;
font-size: $font-size;
line-height: (24px / $font-size); // line-height in px divided by font-size
}

@mixin text-body-small {
$font-size: 14px;
font-weight: 400;
font-size: $font-size;
line-height: (20px / $font-size); // line-height in px divided by font-size
}

@mixin text-button {
$font-size: 14px;
font-weight: 600;
font-size: $font-size;
line-height: (20px / $font-size); // line-height in px divided by font-size
}

@mixin text-caption {
$font-size: 12px;
font-weight: 400;
font-size: $font-size;
line-height: (16px / $font-size); // line-height in px divided by font-size
}

@mixin text-label {
$font-size: 12px;
font-weight: 600;
font-size: $font-size;
line-height: (16px / $font-size); // line-height in px divided by font-size
}

// 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.

Might be a better way to do this. I had to add a class here:

https://github.com/WordPress/gutenberg/pull/18244/files/d59e72aeaab2c548c80abaee74d9c5edba77f805#diff-78ecdbd59f063cfb5365e815d2a58fd7R90

}

/**
* Breakpoint mixins
*/
Expand Down
6 changes: 3 additions & 3 deletions packages/base-styles/_variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@

// Fonts & basics
$default-font: -apple-system, BlinkMacSystemFont,"Segoe UI", Roboto, Oxygen-Sans, Ubuntu, Cantarell,"Helvetica Neue", sans-serif;
$default-font-size: 13px;
$default-line-height: 1.4;
$default-font-size: 14px;
$default-line-height: (20px / $default-font-size); // line-height in px divided by font-size
$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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

$mobile-text-min-font-size: 16px; // Any font size below 16px will cause Mobile Safari to "zoom in"

// Grid size
Expand Down
2 changes: 1 addition & 1 deletion packages/block-directory/src/components/downloadable-block-author-info/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@

.block-directory-downloadable-block-author-info__content-author {
margin-bottom: 4px;
font-size: 14px;
@include text-body-small;
}
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/block-breadcrumb/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,5 @@
.block-editor-block-breadcrumb__current {
color: $dark-gray-500;
padding: 0 $grid-size;
font-size: inherit;
@include text-label;
}
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/block-card/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
}

.block-editor-block-card__title {
font-weight: 500;
margin-bottom: 5px;
@include text-body;
color: $dark-gray-900;
}

.block-editor-block-card__description {
Expand Down
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/block-compare/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,10 @@

.block-editor-block-compare__html {
font-family: $editor-html-font;
font-size: 12px;
@include text-caption;
color: $dark-gray-800;
border-bottom: 1px solid #ddd;
padding-bottom: 15px;
line-height: 1.7;

span {
background-color: #e6ffed;
Expand Down
6 changes: 2 additions & 4 deletions packages/block-editor/src/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1090,18 +1090,16 @@

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

@jameslnewell jameslnewell Nov 5, 2019 •
edited
Loading

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.

font-size: 11px;
@include text-caption;
padding: 4px 4px;
background: $light-gray-500;
color: $dark-gray-900;
transition: box-shadow 0.1s linear;
@include reduce-motion("transition");

.components-button {
font-size: inherit;
line-height: inherit;
@include text-label;
padding: 0;
}

Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/inserter-list-item/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
display: flex;
flex-direction: column;
width: 100%;
font-size: $default-font-size;
@include text-caption;
color: $dark-gray-700;
padding: 0 4px;
align-items: stretch;
Expand Down Expand Up @@ -72,7 +72,7 @@
}

.block-editor-block-types-list__item-icon {
padding: 12px 20px;
padding: 8px 20px;
border-radius: $radius-round-rectangle;
color: $dark-gray-500;
transition: all 0.05s ease-in-out;
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/inserter/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ $block-inserter-search-height: 38px;
align-items: center;

h2 {
font-size: 13px;
@include text-body-small;
}

.block-editor-block-icon {
Expand Down
8 changes: 8 additions & 0 deletions packages/block-editor/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 👍

}

body p {
@include text-body-small;
}
2 changes: 1 addition & 1 deletion packages/block-library/src/block/edit-panel/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@

.reusable-block-edit-panel__title {
flex: 1 1 100%;
font-size: 14px;
@include text-body-small;
height: 30px;
margin: $grid-size-small 0 $grid-size;
}
Expand Down
5 changes: 2 additions & 3 deletions packages/components/src/base-control/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
font-size: $default-font-size;

.components-base-control__field {
margin-bottom: $grid-size;
margin-bottom: $grid-size-small;

.components-panel__row & {
margin-bottom: inherit;
Expand All @@ -16,8 +16,7 @@
}

.components-base-control__help {
margin-top: -$grid-size;
font-style: italic;
@include text-caption;
}
}

Expand Down
13 changes: 5 additions & 8 deletions packages/components/src/button/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
.components-button {
display: inline-flex;
align-items: center;
text-decoration: none;
font-size: $default-font-size;
margin: 0;
border: 0;
cursor: pointer;
-webkit-appearance: none;
background: none;
transition: box-shadow 0.1s linear;
@include text-button;
@include reduce-motion("transition");

&.is-button {
padding: 0 10px;
line-height: 2;
height: 28px;
border-radius: 3px;
white-space: nowrap;
Expand Down Expand Up @@ -193,15 +193,13 @@

&.is-large {
height: 30px;
line-height: 28px;
padding: 0 12px 2px;
padding: 0 12px 0;
}

&.is-small {
height: 24px;
line-height: 22px;
padding: 0 8px 1px;
font-size: 11px;
padding: 0 8px 0;
font-size: 12px;
}

// Buttons that are text-based.
Expand All @@ -210,7 +208,6 @@

// Matches default button in hit area. See line 11.
padding: 0 10px;
line-height: 26px;
height: 28px;

.dashicon {
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/date-time/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@


&.am-pm button {
font-size: 11px;
font-size: 12px;
font-weight: 600;
}

Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/form-token-field/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
flex-wrap: wrap;
align-items: flex-start;
width: 100%;
margin: 0 0 $grid-size 0;
margin: 0 0 $grid-size-small 0;
padding: $grid-size-small;
background-color: $white;
border: $border-width solid $light-gray-700;
Expand Down Expand Up @@ -51,7 +51,7 @@
}

.components-form-token-field__help {
font-style: italic;
@include text-caption;
}

// Tokens
Expand Down
3 changes: 1 addition & 2 deletions packages/components/src/menu-item/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
text-align: left;
color: $dark-gray-600;
@include menu-style__neutral;
@include text-body-small;

// Target plugin icons that can have arbitrary classes by using an aggressive selector.
.dashicon,
Expand Down Expand Up @@ -37,8 +38,6 @@
}

.components-menu-item__info {
margin-top: $grid-size-small;
font-size: $default-font-size - 1px;
color: $dark-gray-300;
}

Expand Down
3 changes: 1 addition & 2 deletions packages/components/src/modal/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,7 @@
}

.components-modal__header-heading {
font-size: 1rem;
font-weight: 600;
@include text-subtitle;
}

h1 {
Expand Down
9 changes: 6 additions & 3 deletions packages/edit-post/src/components/keyboard-shortcut-help-modal/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
kbd.edit-post-keyboard-shortcut-help__shortcut-term {
@include text-body-small;
}

.edit-post-keyboard-shortcut-help {
&__section {
margin: 0 0 2rem 0;
Expand All @@ -9,8 +13,7 @@
}

&__section-title {
font-size: 0.9rem;
font-weight: 600;
@include text-subtitle-small;
}

&__shortcut {
Expand All @@ -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.

margin: 0 0 0 1rem;
}

Expand Down
4 changes: 1 addition & 3 deletions packages/edit-post/src/components/layout/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,6 @@
// Match the size of the Publish... button.
.components-button.is-large {
height: 33px;
line-height: 32px;
}

// Size the spacer flexibly to allow for different button lengths.
Expand Down Expand Up @@ -196,8 +195,7 @@
width: auto;
height: auto;
display: block;
font-size: 14px;
font-weight: 600;
@include text-button;
margin: 0 0 0 auto;
padding: 15px 23px 14px;
line-height: normal;
Expand Down
3 changes: 1 addition & 2 deletions packages/edit-post/src/components/manage-blocks-modal/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@
}

.components-checkbox-control__label {
font-size: 0.9rem;
font-weight: 600;
@include text-subtitle-small;
}
}

Expand Down
3 changes: 1 addition & 2 deletions packages/edit-post/src/components/options-modal/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@
}

&__section-title {
font-size: 0.9rem;
font-weight: 600;
@include text-subtitle-small;
}

&__option {
Expand Down
Loading