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

Twenty Twenty Blocks: Try adding new Global Styles Rules #44

Merged
merged 18 commits into from
Jul 21, 2020

Conversation

kjellr
Copy link
Collaborator

@kjellr kjellr commented Jun 23, 2020

Takes a quick pass at adding Global Styles rules to Twenty Twenty Blocks, as per the new documentation. Essentially an updated version of #26.

@kjellr kjellr requested a review from jffng June 23, 2020 13:56
@kjellr kjellr self-assigned this Jun 23, 2020
@kjellr kjellr added the enhancement New feature or request label Jun 23, 2020
@@ -249,29 +254,30 @@ Inter variable font. Usage:
.editor-styles-wrapper .wp-block h6 {
font-feature-settings: "lnum";
font-variant-numeric: lining-nums;
font-weight: 700;
font-weight: var(--wp--typography--font-weight-heading);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have the --wp--typography-- variables gone away? I don't see them being applied anymore when I activate this theme. Do they need to be converted to presets in the theme config?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, whoops. That's a leftover from the last attempt at this. It needs to be converted to use the new system.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be take care of in fe1e88e.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually wait — I need to get it fixed in the editor styles too. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, should be ok as of a442c52.

twentytwenty-blocks/functions.php Show resolved Hide resolved
@kjellr
Copy link
Collaborator Author

kjellr commented Jul 14, 2020

@jffng This should be ready for another review. 👍

twentytwenty-blocks/twentytwenty-styles/style.css Outdated Show resolved Hide resolved
font-size: 36px;
font-weight: 800;
.editor-styles-wrapper h1.wp-block {
font-size: 84px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering why we're using px values here when rem values are used in the frontend.

Copy link
Collaborator Author

@kjellr kjellr Jul 14, 2020

Choose a reason for hiding this comment

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

In the frontend, Twenty Twenty modifies the value of 1 rem to be 10px instead of the usual 16px. Since that doesn't happen in the editor (and can't really, without messing with a lot of other font sizes), we're stuck with pixels for the moment.

These should really be variables, but I didn't switch to using variables for these sizes because the heading font sizes don't match up with the small, normal, large, + huge sizes. I figured we'd keep things simple for now, and can open up a separate PR improve this.

Copy link
Collaborator

@jffng jffng left a comment

Choose a reason for hiding this comment

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

I left one comment about applying colors to buttons. Other than that, this works in my testing.

background-color: #000;
color: #f5efe0;
background-color: var(--wp--preset--color--text);
color: var(--wp--preset--color--primary);
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is some strangeness when you apply a background color and text color to buttons with the "Fill" style:

Editor
Screen Shot 2020-07-21 at 9 56 20 AM

Front-end
Screen Shot 2020-07-21 at 9 56 26 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to do a few weird things to get this working:

  1. Rename the "Text" color to "Foreground". Otherwise, the class to indicate that this color was being used was has-text-color. This also happens to be the generic classname that's added to blocks when they have any text color assigned. That caused some conflicting styles.
  2. I had to rewrite some of the styles, because they seemed to be wrong.
  3. I had to add some extra specificity to get the custom color rules working for buttons.

It's possible I chopped out a little more code than was necessary here, but it generally seemed to work for me.

@@ -2736,21 +2736,21 @@ h2.entry-title {
/* CUSTOM COLORS */

:root .has-accent-color {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the :root selector still needed? I noticed that in #45 it wasn't used and still worked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, these don't get used at all currently — they're overridden by rules assigned to .editor-styles-wrapper instead.

Copy link
Collaborator

@jffng jffng left a comment

Choose a reason for hiding this comment

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

This works and I think does a good job demonstrating where we're at with the current Global Styles mechanism. Left one comment that's not super relevant but had me head-scratching nonetheless.

@@ -167,48 +167,34 @@ Inter variable font. Usage:

/* CUSTOM COLORS */

:root .has-accent-color {
.editor-styles-wrapper .has-primary-color,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to target .editor-styles-wrapper? I though that enqueueing via add_editor_style was supposed to handle this for us?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, that's a good question. The non-blocks version of twenty twenty enqueues the editor styles using the old hook, not via add_editor_style. Since we don't do that in the block-based theme here, we should probably strip all these out of the editor stylesheet. They're not necessary anymore.

I'll leave that for a followup PR though.

@kjellr kjellr merged commit e709b2e into master Jul 21, 2020
@kjellr kjellr deleted the add/twenty-twenty-global-styles branch July 21, 2020 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants