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

Update the editor styles wrapper to avoid specificity issues #10956

Merged
merged 8 commits into from
Oct 26, 2018

Conversation

youknowriad
Copy link
Contributor

Related #10067

This PR updates the wrapper of the editor styles to the container of the post content. This means:

  • No more specificity issues we want to target blocks
  • Possibility to tweak the appender and the post title using the editor styles.

This could have an impact on the UI (regressions in movers or things like that). I haven't caught any bug yet though. I'd appreciate some help testing this with themes providing Gutenberg editor styles or not. Thanks

@nickcernis
Copy link
Contributor

Looks good. I tested with two themes containing editor styles for Gutenberg and found no major issues with mapping .wp-block / body to the new .editor-styles-wrapper instead of .editor-block-list__block so far.

I also tested styling the editor page title, which works well withbody .editor-post-title__block .editor-post-title__input as the selector, mapping to .editor-styles-wrapper .editor-post-title__block .editor-post-title__input.

screen shot on 2018-10-23 at 17 19 41

Gutenberg UI styling also appears unaffected.

The only thing that broke for me was these rules designed to set wide and full block widths:

body[data-align="wide"] {
	max-width: 1092px !important;
}

body[data-align="full"] {
	max-width: none !important;
}

Which now maps to this, and have no effect on block width:

.editor-styles-wrapper[data-align="wide"] {
max-width: 1092px !important;
}

.editor-styles-wrapper[data-align="full"] {
max-width: none !important;
}

But this is easily fixed by doing this instead:

body .editor-block-list__block[data-align="wide"] {
	max-width: 1092px;
}

body .editor-block-list__block[data-align="full"] {
	max-width: none;
}

Which looks like what we might want to update this section of the guidebook to once this PR is merged; the wide and full styles there will no longer work, as they're mapped to .editor-styles-wrapper.block-editor-page .editor-block-list__block[data-align="wide"] . (The 'main column width' there can just use '.wp-block' once this PR is made.)

@youknowriad
Copy link
Contributor Author

Thanks @nickcernis for the tests. Glad it worked so far.

A thought I have is trying to support settings widths like that:

.wp-block {
   max-width: 600px
}

.wp-block[data-align="wide"] {
	max-width: 1092px;
}

.wp-block[data-align="full"] {
	max-width: none;
}

and instead of doing advanced rules rewriting just apply the wp-block className to:

  • The block's wrapper
  • The block appender
  • The post title

I think it will bring clarity and simplify the process a lot. No magic => better understanding of how things work from theme authors.

(in addition to supporting the legacy $content_width)

cc @jasmussen

@youknowriad youknowriad added this to the 4.2 milestone Oct 23, 2018
@nickcernis
Copy link
Contributor

A thought I have is trying to support settings widths like that…

Yes, that reads much more clearly in terms of the intent. It would be nice to avoid using .editor-block-list__block directly as much as possible.

@chrisvanpatten
Copy link
Contributor

chrisvanpatten commented Oct 23, 2018

This seems great to me. I think the next step is potentially running more CSS through this mechanism, such as the block library styles (where some of the CSS might be less specific and thus not apply because it doesn't account for the .editor-styles-wrapper selector, like .editor-styles-wrapper p is more specific than .has-large-font-size so the font size wouldn't be applied) but this feels like a big step in the right direction.

@mtias mtias added the Customization Issues related to Phase 2: Customization efforts label Oct 23, 2018
@youknowriad
Copy link
Contributor Author

Ok, I went ahead and pushed this proposal #10956 (comment)

Let me know what you think. If all goes well, I'll make sure to update the docs accordingly.

@mtias
Copy link
Member

mtias commented Oct 23, 2018

Great to see a preponderance of red :)

@kjellr
Copy link
Contributor

kjellr commented Oct 23, 2018

This is looking good! I was able to move all of our Twenty Nineteen styles back into the default stylesheet. 👏 🎉

Just a couple notes:

  • I had to use some really specific rules to override the font for the default inserter. The rule I had to override was specifying .editor-default-block-appender input[type="text"].editor-default-block-appender__content.
  • To target specific blocks with different alignments, I had to specify .wp-block[data-type="core/cover"][data-align="full"]. This would be cleaner if we could just use .wp-block-cover[data-align="full"] (or even .wp-block-cover.alignfull to match up with the front end for that matter 😉 )

@youknowriad
Copy link
Contributor Author

@kjellr Glad it worked well 👍

I had to use some really specific rules to override the font for the default inserter. The rule I had to override was specifying .editor-default-block-appender input[type="text"].editor-default-block-appender__content

I think this can be improved. There's a lot of places where we can improve specificity and this is one of them. Maybe we should create a new label "Editor Styles" and create discrete issues to track these quick wins.

To target specific blocks with different alignments,

Unfortunately, this is not possible, there are some technical concerns in the editor view forcing us to have an extra "div" around blocks.

@youknowriad youknowriad force-pushed the update/editor-style-wrapper branch from 5cd8e7a to 6908f13 Compare October 24, 2018 10:50
kjellr added a commit to WordPress/twentynineteen that referenced this pull request Oct 24, 2018
A followup to #22.

This PR removes our secondary `style-editor-frame.css` file, and consolidates all of our editor styles into `style-editor.css`. Initially, the standard editor stylesheet was limited in scope to individual blocks, which meant we were unable to style the general alignment of the page, as well as items like the page title and default appender.

The scope of `style-editor.css` is being expanded in WordPress/gutenberg#10956 , so we're now able to style those elements in a single stylesheet.

Some minor style tweaks were necessary to make our original styles work in this context, but for the most part this is identical to #22.
@youknowriad
Copy link
Contributor Author

I updated the docs, I removed the old way of theming the editor. (cc @jasmussen )

This PR should be ready for a final review.

@chrisvanpatten
Copy link
Contributor

@youknowriad This definitely makes sense as the path going forward! Will enqueue_block_editor_assets get a deprecation warning or will that continue to be supported?

@youknowriad
Copy link
Contributor Author

@chrisvanpatten 🤔 I don't mind keeping it for some advanced use-cases where you want to style outside of the box, I think.

@chrisvanpatten
Copy link
Contributor

Just an FYI I'm planning to give this a proper review in the next 24 hours (maaaaybe tonight?) while also converting a client project from the old enqueue hook to add_editor_style 😁

Copy link
Contributor

@chrisvanpatten chrisvanpatten left a comment

Choose a reason for hiding this comment

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

Overall good, just a few documentation things.

One general note is that I got pretty tripped up by the add_theme_support('editor-styles'); bit. I was confused because add_editor_style calls add_theme_support('editor-style'); in the function. I didn't notice the difference between editor-styles and editor-style.

I hope my documentation change made it a little clearer that the extra add_theme_support call is necessary :)

In practice, this was really nice once I figured that out. Excited to see it land!

docs/extensibility/theme-support.md Outdated Show resolved Hide resolved
packages/edit-post/src/style.scss Outdated Show resolved Hide resolved
Co-Authored-By: youknowriad <benguella@gmail.com>
@chrisvanpatten
Copy link
Contributor

My same suggestion also applies from up the thread, that we use this method to wrap the block library styles as well, so editor styles and block styles operate at the same level of selector specificity.

That could be done in a separate PR but I think it's essential that block styles and editor styles are as similarly-scoped as possible.

@youknowriad youknowriad merged commit ce8f61d into master Oct 26, 2018
@youknowriad youknowriad deleted the update/editor-style-wrapper branch October 26, 2018 13:36
@samikeijonen
Copy link
Contributor

samikeijonen commented Oct 26, 2018

I'd definitely keep enqueue_block_editor_assets. That's still the hook where I enqueue styles for the editor. Not all of us is using add_editor_style .

@chrisvanpatten
Copy link
Contributor

chrisvanpatten commented Oct 26, 2018

@samikeijonen Out of curiosity is there a reason you're going with that method instead of add_editor_style? I've found the auto-transformation to be very reliable and helpful.

@nickcernis
Copy link
Contributor

@chrisvanpatten I use enqueue_block_editor_assets to enqueue custom fonts specific to the Gutenberg editor. I think that's a valid use case too?

@samikeijonen
Copy link
Contributor

@chrisvanpatten Mostly because I'm a control freak:) And with add_editor_style I kind of lost the control. I know it has improved a lot and might switch to that again at some point.


```php
add_theme_support( 'editor-styles' );
add_theme_support('editor-styles');
Copy link
Member

Choose a reason for hiding this comment

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

Why the removal of spaces?

Copy link
Contributor

Choose a reason for hiding this comment

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

That’s my fault. I code in PSR-2 and forgot to adjust in my comment. I’ll make a note to fix it.

@m-e-h
Copy link
Member

m-e-h commented Nov 16, 2018

Not sure if this is best addressed with the button-preview classes or the body wrapper replacement. #11997

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customization Issues related to Phase 2: Customization efforts [Feature] Custom Editor Styles Functionality for adding custom editor styles [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants