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

Styles: add Editor Styles support #9008

Merged
merged 17 commits into from Sep 5, 2018

Conversation

@youknowriad
Contributor

youknowriad commented Aug 15, 2018

Extending the editor styles is not an easy task for theme developers. They have to take care of all the specifity that Gutenberg styles use. And it's prone to errors because Gutenberg can change the wrappers at any time.

In this PR I'm exploring auto-injecting namespaces (specifity) for theme CSS to avoid leaving that to the theme developpers.

A theme author would write:

h2 { color: red; }

The injected CSS would be

.editor-block-list__block h2 { color: red; }

To test it, notice that the editor styles do work for the Twenty Seventeen theme.

@youknowriad youknowriad self-assigned this Aug 15, 2018

@youknowriad youknowriad requested review from mtias, jasmussen and WordPress/gutenberg-core Aug 15, 2018

@tofumatt

I think this is pretty cool as a POC! I haven't tried to make a theme so I'm not sure what edge cases this would affect, but the approach makes sense to me. 👍

@tofumatt tofumatt requested a review from WordPress/gutenberg-core Aug 15, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 15, 2018

Contributor

Can confirm the heading is red:

screen shot 2018-08-15 at 11 35 54

Nice work, very interesting. Given that the editing canvas is neither inside its own iframe, nor utilizing Shadow DOM in any way, this seems like a promising alternative way to simplify the instructions here: https://wordpress.org/gutenberg/handbook/extensibility/theme-support/#editor-styles

However this would require a build process for themers, no? Is there a way we can do this on the fly in Gutenberg whenever a theme declares it has an editor style for Gutenberg?

Overall I think this could be a positive step in the right direction. But as we simplify these aspects, other challenges present themselves. Notably the fact that the editor markup is wildly different from the front-end markup. This is also discussed here: #8350 (comment), where I use the quote as an example.

Where it gets tricky is when nested blocks are introduced. Take the table, for example. This is basic table markup:

<table>
	<tr>
		<td></td>
	</tr>
	<tr>
		<td></td>
	</tr>
</table>

If we add nesting support to this block, then the table itself will become one block, the table row another, and the table cell a third. There might still be tr's and td's buried in there, but in the editing canvas every one of those elements is now wrapped in a bunch of extra elements necessary for the nesting to function.

We've been able to work around this so far, but at some point we'll likely have to tackle this. Would Shadow DOM help here? Can we use display: contents; to simulate passthrough here?

Contributor

jasmussen commented Aug 15, 2018

Can confirm the heading is red:

screen shot 2018-08-15 at 11 35 54

Nice work, very interesting. Given that the editing canvas is neither inside its own iframe, nor utilizing Shadow DOM in any way, this seems like a promising alternative way to simplify the instructions here: https://wordpress.org/gutenberg/handbook/extensibility/theme-support/#editor-styles

However this would require a build process for themers, no? Is there a way we can do this on the fly in Gutenberg whenever a theme declares it has an editor style for Gutenberg?

Overall I think this could be a positive step in the right direction. But as we simplify these aspects, other challenges present themselves. Notably the fact that the editor markup is wildly different from the front-end markup. This is also discussed here: #8350 (comment), where I use the quote as an example.

Where it gets tricky is when nested blocks are introduced. Take the table, for example. This is basic table markup:

<table>
	<tr>
		<td></td>
	</tr>
	<tr>
		<td></td>
	</tr>
</table>

If we add nesting support to this block, then the table itself will become one block, the table row another, and the table cell a third. There might still be tr's and td's buried in there, but in the editing canvas every one of those elements is now wrapped in a bunch of extra elements necessary for the nesting to function.

We've been able to work around this so far, but at some point we'll likely have to tackle this. Would Shadow DOM help here? Can we use display: contents; to simulate passthrough here?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 15, 2018

Contributor

So this doesn't require any build step, it works on the fly. Locally I have a commit enabling this for the old "editor styles".

This surfaces some issues

  • sometimes these theme styles do things like body { rule } or body selector { rule }. Since I’m just adding a namespace, these styles won’t be applied.
  • the stylesheets are converted on the fly, which mean the relative links (images,...) in the styles won’t work at the moment.

I believe both of these issues can be fixed using:

1- More smart injecting mechanism. If we detect that the selector body in the selector, we inject the namespace after that
2- Since the CSS should be read and injected, we need to rewrite the relative urls on the fly to inject the right prefixes on the fly.

Alternatively, we can build a dedicated "gutenberg" API and avoid supporting the old editor styles for now.

Contributor

youknowriad commented Aug 15, 2018

So this doesn't require any build step, it works on the fly. Locally I have a commit enabling this for the old "editor styles".

This surfaces some issues

  • sometimes these theme styles do things like body { rule } or body selector { rule }. Since I’m just adding a namespace, these styles won’t be applied.
  • the stylesheets are converted on the fly, which mean the relative links (images,...) in the styles won’t work at the moment.

I believe both of these issues can be fixed using:

1- More smart injecting mechanism. If we detect that the selector body in the selector, we inject the namespace after that
2- Since the CSS should be read and injected, we need to rewrite the relative urls on the fly to inject the right prefixes on the fly.

Alternatively, we can build a dedicated "gutenberg" API and avoid supporting the old editor styles for now.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 15, 2018

Contributor

So this surfaced something else:

To make some of the Gutenberg default styles overridable, we should use the exact same specificity, this transformation uses.

For example, in gutenberg we do this to customize the editor's font family

.edit-post-visual-editor, .edit-post-visual-editor p {
  font-family: "Noto Serif", serif;
}

We should update this to support themes extending the default font.

.editor-block-list__block {
  font-family: "Noto Serif", serif;
}

After this change, a theme would be able to provide

body {
  font-family: "my custom font" 
}

and it will work.

Contributor

youknowriad commented Aug 15, 2018

So this surfaced something else:

To make some of the Gutenberg default styles overridable, we should use the exact same specificity, this transformation uses.

For example, in gutenberg we do this to customize the editor's font family

.edit-post-visual-editor, .edit-post-visual-editor p {
  font-family: "Noto Serif", serif;
}

We should update this to support themes extending the default font.

.editor-block-list__block {
  font-family: "Noto Serif", serif;
}

After this change, a theme would be able to provide

body {
  font-family: "my custom font" 
}

and it will work.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 20, 2018

Contributor

I updated the PR to:

  • Fix the body selectors issues
  • Fix the relative URLs issue (using a custom post CSS plugin)
  • Support the current theme's editor styles.

I'd appreciate some eyes on this PR, to ensure

  • Current themes with editor styles don't break Gutenberg's styling :)
  • Some of these editor styles are applied properly (we don't want to support everything, but basic support should work)

Also, I'd appreciate thoughts on the approach.

Contributor

youknowriad commented Aug 20, 2018

I updated the PR to:

  • Fix the body selectors issues
  • Fix the relative URLs issue (using a custom post CSS plugin)
  • Support the current theme's editor styles.

I'd appreciate some eyes on this PR, to ensure

  • Current themes with editor styles don't break Gutenberg's styling :)
  • Some of these editor styles are applied properly (we don't want to support everything, but basic support should work)

Also, I'd appreciate thoughts on the approach.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 20, 2018

Contributor

I refactored the Gutenberg default styles to make this #9008 (comment) possible

You can see that if you use The twenty* themes for instance, there's basic support for editor styles.

Contributor

youknowriad commented Aug 20, 2018

I refactored the Gutenberg default styles to make this #9008 (comment) possible

You can see that if you use The twenty* themes for instance, there's basic support for editor styles.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 20, 2018

Contributor

Noting that bundling postCSS in the browser does add some Kb (280). Do you think it's worth it? how can we improve it?

Contributor

youknowriad commented Aug 20, 2018

Noting that bundling postCSS in the browser does add some Kb (280). Do you think it's worth it? how can we improve it?

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 20, 2018

Contributor

To make some of the Gutenberg default styles overridable, we should use the exact same specificity, this transformation uses.

Yes, agreed. Might even consider doing these on the fly as well for our editor styles (basically assume we have a shadow dom when we write our CSS).

Contributor

mtias commented Aug 20, 2018

To make some of the Gutenberg default styles overridable, we should use the exact same specificity, this transformation uses.

Yes, agreed. Might even consider doing these on the fly as well for our editor styles (basically assume we have a shadow dom when we write our CSS).

@gziolo

Noting that bundling postCSS in the browser does add some Kb (300). Do you think it's worth it? how can we improve it?

That's a lot. At the moment we are at 1.2-1.3 MB minified and gzipped JavaScript code served to the users. Adding another 300 kB doesn't seem like a good idea. Should we look into some CSS in JS alternatives?

Show outdated Hide outdated packages/postcss-url/src/index.js
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 20, 2018

Contributor

Should we look into some CSS in JS alternatives?

What alternatives do you think of? Ideally, this could be achieved in PHP but I feel rewriting something like PostCSS in PHP (5.2, a rewrite in recent versions exist) seems overkill? We could do a regexp based parser but doesn't seem like a solid solution.

If we were to stick with JavaScript, we can try to build a lighter version of this by looping through the styles with regex or something else, but it doesn't feel solid either or would take a lot of time to make it work properly in all cases.

I'm open to alternatives, but it does feel like a lot of work to achieve the same thing.

Contributor

youknowriad commented Aug 20, 2018

Should we look into some CSS in JS alternatives?

What alternatives do you think of? Ideally, this could be achieved in PHP but I feel rewriting something like PostCSS in PHP (5.2, a rewrite in recent versions exist) seems overkill? We could do a regexp based parser but doesn't seem like a solid solution.

If we were to stick with JavaScript, we can try to build a lighter version of this by looping through the styles with regex or something else, but it doesn't feel solid either or would take a lot of time to make it work properly in all cases.

I'm open to alternatives, but it does feel like a lot of work to achieve the same thing.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 21, 2018

Contributor

I'm not against the added weight if it solves some very important issues in an elegant way.

Contributor

mtias commented Aug 21, 2018

I'm not against the added weight if it solves some very important issues in an elegant way.

@simison

This comment has been minimized.

Show comment
Hide comment
@simison

simison Aug 22, 2018

We could do a regexp based parser but doesn't seem like a solid solution.

postscss-prefixwrap really is just a bunch of regexp so might be there's a way to make this more lightweight?

I've been working on a similar issue in Automattic/wp-calypso#26683 Instead of postcss-prefixwrap, I've been looking into namespace-css. It uses CSS AST to modify CSS, instead of regexp which might be expensive for big stylesheets and can be error-prone.

Some of the shortcomings I learned were that these prefixing libraries don't handle well:

  • body and html (it just prefixes them to .prefix body {} instead of renaming them to .prefix {})
  • @keyframes stay as is thus making name collisions possible
  • Font imports
  • ::selection
  • @page

simison commented Aug 22, 2018

We could do a regexp based parser but doesn't seem like a solid solution.

postscss-prefixwrap really is just a bunch of regexp so might be there's a way to make this more lightweight?

I've been working on a similar issue in Automattic/wp-calypso#26683 Instead of postcss-prefixwrap, I've been looking into namespace-css. It uses CSS AST to modify CSS, instead of regexp which might be expensive for big stylesheets and can be error-prone.

Some of the shortcomings I learned were that these prefixing libraries don't handle well:

  • body and html (it just prefixes them to .prefix body {} instead of renaming them to .prefix {})
  • @keyframes stay as is thus making name collisions possible
  • Font imports
  • ::selection
  • @page
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 22, 2018

Contributor

The reason I chose postscss-prefixwrap is because it handles body properly. It does exactly that #9008 (comment)

And the bundle size increase is not because of postscss-prefixwrap but it's because of postcss which itself also works on ASTs. The other drawbacks of namespacing are acceptable IMO.

Also, namespacing is not the only thing we need here, we also need to rewrite the URLs because we're not including the stylesheet by URL, we're reading it and including it in any random webpage, so we need to use absolute URLs.

Contributor

youknowriad commented Aug 22, 2018

The reason I chose postscss-prefixwrap is because it handles body properly. It does exactly that #9008 (comment)

And the bundle size increase is not because of postscss-prefixwrap but it's because of postcss which itself also works on ASTs. The other drawbacks of namespacing are acceptable IMO.

Also, namespacing is not the only thing we need here, we also need to rewrite the URLs because we're not including the stylesheet by URL, we're reading it and including it in any random webpage, so we need to use absolute URLs.

@simison

This comment has been minimized.

Show comment
Hide comment
@simison

simison Aug 22, 2018

And the bundle size increase is not because of postscss-prefixwrap but it's because of postcss which itself also works on ASTs.

Sure, I was thinking that ditching postscss requirement would bring the size down but if there are other use cases for it than prefixing, it indeed does make sense to keep it. 👍

simison commented Aug 22, 2018

And the bundle size increase is not because of postscss-prefixwrap but it's because of postcss which itself also works on ASTs.

Sure, I was thinking that ditching postscss requirement would bring the size down but if there are other use cases for it than prefixing, it indeed does make sense to keep it. 👍

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 22, 2018

Contributor

I wonder if using just CSS AST and looping through the rules would work, we'd need to recreate CSS namespacing and URL replacement our selves though. Worth a try.

Contributor

youknowriad commented Aug 22, 2018

I wonder if using just CSS AST and looping through the rules would work, we'd need to recreate CSS namespacing and URL replacement our selves though. Worth a try.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 23, 2018

Contributor

I updated the PR to use the CSS AST approach instead of PostCSS. We gain something like 200Kb (Thanks @simison for the hint). That said there are some issues with the css dependency.

It uses fs which makes it node dependent. The thing is, we don't need fs because it's only used in source maps generation which we don't use at all. So I just added fs to our webpack config as external. We also have some warnings when we run the build (due to the sourcemaps too), So I was wondering how to proceed. I'm leaning towards forking the css dependency inside Gutenberg and just remove the sourcemap handling entirely. Thoughts (@mtias @gziolo @aduth @simison )

Contributor

youknowriad commented Aug 23, 2018

I updated the PR to use the CSS AST approach instead of PostCSS. We gain something like 200Kb (Thanks @simison for the hint). That said there are some issues with the css dependency.

It uses fs which makes it node dependent. The thing is, we don't need fs because it's only used in source maps generation which we don't use at all. So I just added fs to our webpack config as external. We also have some warnings when we run the build (due to the sourcemaps too), So I was wondering how to proceed. I'm leaning towards forking the css dependency inside Gutenberg and just remove the sourcemap handling entirely. Thoughts (@mtias @gziolo @aduth @simison )

/**
* @const string IS_ROOT_TAG Regex to check if the selector is a root tag selector.
*/
const IS_ROOT_TAG = /^(body|html).*$/;

This comment has been minimized.

@simison

simison Aug 23, 2018

To catch also:

body#test {}
body.test {}

Maybe something like this?

const IS_ROOT_TAG = /^(body|html)(?=\s|$|\.|\#)/;
@simison

simison Aug 23, 2018

To catch also:

body#test {}
body.test {}

Maybe something like this?

const IS_ROOT_TAG = /^(body|html)(?=\s|$|\.|\#)/;

This comment has been minimized.

@youknowriad

youknowriad Aug 23, 2018

Contributor

I'm not certain about this, at least not in Gutenberg's use-case because. It doesn't mean anything to do body.test or body#test for editor styles.

@youknowriad

youknowriad Aug 23, 2018

Contributor

I'm not certain about this, at least not in Gutenberg's use-case because. It doesn't mean anything to do body.test or body#test for editor styles.

This comment has been minimized.

@simison

simison Aug 24, 2018

Should we rename body in body[dir='rtl'] though?

@simison

simison Aug 24, 2018

Should we rename body in body[dir='rtl'] though?

This comment has been minimized.

@youknowriad

youknowriad Aug 24, 2018

Contributor

That's a good question and even if we don't rename it, it won't work anyway. Because there's no dir attribute on the wrapper where we apply the styles. I guess this should be addressed separately.

@youknowriad

youknowriad Aug 24, 2018

Contributor

That's a good question and even if we don't rename it, it won't work anyway. Because there's no dir attribute on the wrapper where we apply the styles. I guess this should be addressed separately.

This comment has been minimized.

@simison

simison Aug 24, 2018

Do you know if there's anything in Gutenberg setting dir? If there is, renaming it to [dir='rtl'] .wrapper (with space) could work?

@simison

simison Aug 24, 2018

Do you know if there's anything in Gutenberg setting dir? If there is, renaming it to [dir='rtl'] .wrapper (with space) could work?

This comment has been minimized.

@youknowriad

youknowriad Aug 24, 2018

Contributor

Yes, there's probably a class or attribute on the body like any other WPAdmin page which means we should introduce a custom "transform" to achieve it.

@youknowriad

youknowriad Aug 24, 2018

Contributor

Yes, there's probably a class or attribute on the body like any other WPAdmin page which means we should introduce a custom "transform" to achieve it.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 23, 2018

Contributor

Ok, So I forked css inside Gutenberg to move forward until we came up with a better option.

Contributor

youknowriad commented Aug 23, 2018

Ok, So I forked css inside Gutenberg to move forward until we came up with a better option.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 23, 2018

Contributor

This PR also make some things possible:

  • For example: we always struggle to define the editor's width. With this PR we have the infrastructure to do smart things like this:

The theme authors write:

body { 
  width: 800px;
}

body.is-wide {
  width: 1000px
}

and we can automatically generate the stylesheet needed to make it work properly https://wordpress.org/gutenberg/handbook/extensibility/theme-support/#changing-the-width-of-the-editor


Other things we could support in a "smart" way: Tweaking the styles of the "title", the background of the editor.

Contributor

youknowriad commented Aug 23, 2018

This PR also make some things possible:

  • For example: we always struggle to define the editor's width. With this PR we have the infrastructure to do smart things like this:

The theme authors write:

body { 
  width: 800px;
}

body.is-wide {
  width: 1000px
}

and we can automatically generate the stylesheet needed to make it work properly https://wordpress.org/gutenberg/handbook/extensibility/theme-support/#changing-the-width-of-the-editor


Other things we could support in a "smart" way: Tweaking the styles of the "title", the background of the editor.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 23, 2018

Contributor

I added some documentation about the new editor styles behavior. Let me know what's missing here.

Contributor

youknowriad commented Aug 23, 2018

I added some documentation about the new editor styles behavior. Let me know what's missing here.

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Aug 23, 2018

@youknowriad youknowriad changed the title from Styles: auto-wrap theme provided styles to avoid specifity in the theme stylesheets to Styles: add Editor Styles support Aug 23, 2018

@simison

This comment has been minimized.

Show comment
Hide comment
@simison

simison Aug 23, 2018

Modifying CSS AST is the way to go 👍 — you get a lot of control over CSS and don't need to carry over added weight from postCSS.

I think forking is the right approach, too, considering that css doesn't seem very active anymore and it has some long-standing open bugs.

simison commented Aug 23, 2018

Modifying CSS AST is the way to go 👍 — you get a lot of control over CSS and don't need to carry over added weight from postCSS.

I think forking is the right approach, too, considering that css doesn't seem very active anymore and it has some long-standing open bugs.

@aduth

What came of the approach of using document.styleSheets to iterate and modify the style properties? This approach of parsing the CSS string seems much more heavy-handed, both in heft of the code size of the parser, and in the performance cost of evaluating the string.

I'm vaguely recalling some previous issue discussing edge cases about various types of blocks. I will return to comment if I can find them again.

A theme author would write:

h2 { color: red; }
The injected CSS would be

.editor-block-list__block h2 { color: red; }

For those block developers who have already written the latter, do we leave it as-is?

Show outdated Hide outdated docs/reference/faq.md
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 23, 2018

Contributor

For those block developers who have already written the latter, do we leave it as-is?

They won't be affected because this is a separate API. This works on the regular editor styles while people that already added those namespaces would have used a separate stylesheet. Both approaches can work together.

Contributor

youknowriad commented Aug 23, 2018

For those block developers who have already written the latter, do we leave it as-is?

They won't be affected because this is a separate API. This works on the regular editor styles while people that already added those namespaces would have used a separate stylesheet. Both approaches can work together.

@ZebulanStanphill

This comment has been minimized.

Show comment
Hide comment
@ZebulanStanphill

ZebulanStanphill Aug 23, 2018

Contributor

So out of curiosity I tried this branch with my website running the Divi theme and this is what it looks like:

image

image

Here is what it actually looks like on the front-end:

image

So I guess this branch has a long way to go. 😛

Contributor

ZebulanStanphill commented Aug 23, 2018

So out of curiosity I tried this branch with my website running the Divi theme and this is what it looks like:

image

image

Here is what it actually looks like on the front-end:

image

So I guess this branch has a long way to go. 😛

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 23, 2018

Contributor

So I guess this branch has a long way to go

Probably not, our goal is to make it work for future themes essentially. We can disable it for existing themes if necessary because as much as I would love them to work without changes, it's not possible because they are built for a different system.

That said, it would be good to help check what's causing these extra spaces. Also the idea is that it can be seen as progressive enhancement for existing themes and not something meant to be 100% compatible.

Contributor

youknowriad commented Aug 23, 2018

So I guess this branch has a long way to go

Probably not, our goal is to make it work for future themes essentially. We can disable it for existing themes if necessary because as much as I would love them to work without changes, it's not possible because they are built for a different system.

That said, it would be good to help check what's causing these extra spaces. Also the idea is that it can be seen as progressive enhancement for existing themes and not something meant to be 100% compatible.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 4, 2018

Contributor

I rebased and added support for tweaking the editor width. So right now you can do:

html {
   width: 800px;
}

And this will update the content area width properly.

Contributor

youknowriad commented Sep 4, 2018

I rebased and added support for tweaking the editor width. So right now you can do:

html {
   width: 800px;
}

And this will update the content area width properly.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 4, 2018

Contributor

@youknowriad Does that work with media queries too? Huge game changer if so!

Contributor

chrisvanpatten commented Sep 4, 2018

@youknowriad Does that work with media queries too? Huge game changer if so!

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 4, 2018

Contributor

@chrisvanpatten Which part are you referring to, the editor width?

Contributor

youknowriad commented Sep 4, 2018

@chrisvanpatten Which part are you referring to, the editor width?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 4, 2018

Contributor

Does that work with media queries too? Huge game changer if so!

I don't think it will :( this would still require the editing canvas being inside an iframe, or possibly in a shadow DOM.

In other words — any media query you write for a theme will not calculate just the width of the editing canvas, the sidebars will also be taken into account.

Contributor

jasmussen commented Sep 4, 2018

Does that work with media queries too? Huge game changer if so!

I don't think it will :( this would still require the editing canvas being inside an iframe, or possibly in a shadow DOM.

In other words — any media query you write for a theme will not calculate just the width of the editing canvas, the sidebars will also be taken into account.

@@ -0,0 +1,685 @@
// Adapted from https://github.com/reworkcss/css
// because we needed to remove source map support.

This comment has been minimized.

@dmsnell

dmsnell Sep 4, 2018

Contributor

did we consider creating an issue or PR on the upstream project to allow things like disabling source maps? it appears like there are a few related issues concerning this…

even if we created an actual fork of the project then included the fork here it could stand a better chance of merging upstream to the point where we could remove the fork later, no?

@dmsnell

dmsnell Sep 4, 2018

Contributor

did we consider creating an issue or PR on the upstream project to allow things like disabling source maps? it appears like there are a few related issues concerning this…

even if we created an actual fork of the project then included the fork here it could stand a better chance of merging upstream to the point where we could remove the fork later, no?

This comment has been minimized.

@youknowriad

youknowriad Sep 4, 2018

Contributor

Yes, good call. I forgot to do that. I'll create an issue upstream, not certain how can they support this but yeah I'll open an issue and see how to move forward depending on the feedback.

@youknowriad

youknowriad Sep 4, 2018

Contributor

Yes, good call. I forgot to do that. I'll create an issue upstream, not certain how can they support this but yeah I'll open an issue and see how to move forward depending on the feedback.

@mtias

mtias approved these changes Sep 4, 2018

This is looking like a great baseline from which to build upon to me. Let's do some rounds of testing (browsers, etc) to make sure there's nothing weird being introduced.

@haszari

This comment has been minimized.

Show comment
Hide comment
@haszari

haszari Sep 4, 2018

What's the best way to test this PR? We'd love to make the Gutenberg editing experience more consistent with the front end on WCCOM.

I'm not clear on best practice for supporting Gutenberg in a theme, other than the info the handbook: https://wordpress.org/gutenberg/handbook/extensibility/theme-support/#editor-styles

How does this PR enhance/improve or change the experience and capabilities for theme developers (of new or existing themes)?

haszari commented Sep 4, 2018

What's the best way to test this PR? We'd love to make the Gutenberg editing experience more consistent with the front end on WCCOM.

I'm not clear on best practice for supporting Gutenberg in a theme, other than the info the handbook: https://wordpress.org/gutenberg/handbook/extensibility/theme-support/#editor-styles

How does this PR enhance/improve or change the experience and capabilities for theme developers (of new or existing themes)?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 5, 2018

Contributor

@haszari See the changes to the page you link to in this PR

In summary. you can define a stylesheet like this https://github.com/WordPress/gutenberg/blob/c58f0eb98bd9ffab76607f1bf3cef5f76d5dc635/packages/editor/src/editor-styles.scss (like the old editor styles file)

and do add_theme_support( 'editor-styles' ); in your theme to load the editor styles in Gutenberg as well.

In these editor styles, you can also target blocks directly without any wrapper .wp-block-quote { } ...

Contributor

youknowriad commented Sep 5, 2018

@haszari See the changes to the page you link to in this PR

In summary. you can define a stylesheet like this https://github.com/WordPress/gutenberg/blob/c58f0eb98bd9ffab76607f1bf3cef5f76d5dc635/packages/editor/src/editor-styles.scss (like the old editor styles file)

and do add_theme_support( 'editor-styles' ); in your theme to load the editor styles in Gutenberg as well.

In these editor styles, you can also target blocks directly without any wrapper .wp-block-quote { } ...

@youknowriad youknowriad merged commit 4fec0d2 into master Sep 5, 2018

2 checks passed

codecov/project 50.96% (+0.58%) compared to 82442fe
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the try/auto-wrap-theme-css branch Sep 5, 2018

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 5, 2018

Contributor

This is not perfect yet. But let's get it in and iterate. Thanks all

Contributor

youknowriad commented Sep 5, 2018

This is not perfect yet. But let's get it in and iterate. Thanks all

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 5, 2018

Contributor

@jasmussen There is a mechanism to set the width in an easier way but not yet the wide and full widths. I don't know yet about the colors.

So my thinking is not yet, we should iterate, test this in some themes and we'll update once we have the final picture.

Contributor

youknowriad commented Sep 5, 2018

@jasmussen There is a mechanism to set the width in an easier way but not yet the wide and full widths. I don't know yet about the colors.

So my thinking is not yet, we should iterate, test this in some themes and we'll update once we have the final picture.

@haszari

This comment has been minimized.

Show comment
Hide comment
@haszari

haszari Sep 5, 2018

@youknowriad Thanks for explaining how this works, I'll give it another try soon :)

haszari commented Sep 5, 2018

@youknowriad Thanks for explaining how this works, I'll give it another try soon :)

@haszari

This comment has been minimized.

Show comment
Hide comment
@haszari

haszari Sep 6, 2018

@youknowriad Update – tried this out and it's working for me, thanks! This should open up lots more possibilities for us :)

haszari commented Sep 6, 2018

@youknowriad Update – tried this out and it's working for me, thanks! This should open up lots more possibilities for us :)

@gutterberg

This comment has been minimized.

Show comment
Hide comment
@gutterberg

gutterberg Sep 12, 2018

I've installed the latest Gutenberg update and this is a massive step in the right direction, though it renders the countless hours I spend getting me theme and Gutenberg to match a complete waste of my life. Oh well.

Unfortunately there are still parts that are massively broken, like not being able to resize images beyond 580px withing the editor: #9302

I've seen no action on this, and it's directly related to being able to make the editor match themes. Whoever thought hard coding anything to 580px was out to lunch, so it need to be fixed.

gutterberg commented Sep 12, 2018

I've installed the latest Gutenberg update and this is a massive step in the right direction, though it renders the countless hours I spend getting me theme and Gutenberg to match a complete waste of my life. Oh well.

Unfortunately there are still parts that are massively broken, like not being able to resize images beyond 580px withing the editor: #9302

I've seen no action on this, and it's directly related to being able to make the editor match themes. Whoever thought hard coding anything to 580px was out to lunch, so it need to be fixed.

@kadamwhite

This comment has been minimized.

Show comment
Hide comment
@kadamwhite

kadamwhite Sep 13, 2018

First off, this is awesome work! I love the idea.

However, because of the mechanism by which styles are detected and processed, it seems like this completely rules out using a webpack dev server or hot-reloading when developing our styles, as there's no CSS file enqueued for Gutenberg to detect and parse and because new CSS updates may be getting injected periodically. Has anybody else encountered this, and is there any thought about providing a mechanism to permit usage of modern dev server tooling when developing Gutenberg editor styles? Forgive me if I've overlooked something.

kadamwhite commented Sep 13, 2018

First off, this is awesome work! I love the idea.

However, because of the mechanism by which styles are detected and processed, it seems like this completely rules out using a webpack dev server or hot-reloading when developing our styles, as there's no CSS file enqueued for Gutenberg to detect and parse and because new CSS updates may be getting injected periodically. Has anybody else encountered this, and is there any thought about providing a mechanism to permit usage of modern dev server tooling when developing Gutenberg editor styles? Forgive me if I've overlooked something.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 13, 2018

Contributor

@kadamwhite I understand the pain here but unfortunately, I can't see how we can make it work with Hot reloading. I'd suggest just reloading the page instead of "hot reloading" when building editor styles.

Contributor

youknowriad commented Sep 13, 2018

@kadamwhite I understand the pain here but unfortunately, I can't see how we can make it work with Hot reloading. I'd suggest just reloading the page instead of "hot reloading" when building editor styles.

@fklein-lu

This comment has been minimized.

Show comment
Hide comment
@fklein-lu

fklein-lu Sep 14, 2018

While I really like this feature, I'm aware that it is not a long term viable solution. Over the medium term, theme developers will just use the build pipeline to generate the editor styles.

To me editor styles are similar to RTL styles. The easiest way to do this is to take the stylesheet, flip it with CSSJanus, enqueue that, and then enqueue a second stylesheet that overrides the rules of the automatically generated stylesheet that are not completely valid.

So once we have a build pipeline plugin for GB editor styles, it can be integrated into the normal development workflow, and you'll then have hot reloading again.

fklein-lu commented Sep 14, 2018

While I really like this feature, I'm aware that it is not a long term viable solution. Over the medium term, theme developers will just use the build pipeline to generate the editor styles.

To me editor styles are similar to RTL styles. The easiest way to do this is to take the stylesheet, flip it with CSSJanus, enqueue that, and then enqueue a second stylesheet that overrides the rules of the automatically generated stylesheet that are not completely valid.

So once we have a build pipeline plugin for GB editor styles, it can be integrated into the normal development workflow, and you'll then have hot reloading again.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 14, 2018

Contributor

@fklein-lu This definitely can be achieved at build time but while some developpers have a build step, others don't and don't want to learn how to do so especially theme developpers. We can't force developers to use build tools. That's why we opted for a runtime solution.

Contributor

youknowriad commented Sep 14, 2018

@fklein-lu This definitely can be achieved at build time but while some developpers have a build step, others don't and don't want to learn how to do so especially theme developpers. We can't force developers to use build tools. That's why we opted for a runtime solution.

@kadamwhite

This comment has been minimized.

Show comment
Hide comment
@kadamwhite

kadamwhite Sep 14, 2018

On the go, so apologies if I’ve missed this while skimming the code: but what about making it filterable, so a dev who wanted a build step could turn this off on a specific big project?

We could then release a canonical package that includes a postcss transform or some such that could be used to decorate a CSS file with the same wrapper classes, providing the same basic benefit for the smaller subset of developers who want a build system.

kadamwhite commented Sep 14, 2018

On the go, so apologies if I’ve missed this while skimming the code: but what about making it filterable, so a dev who wanted a build step could turn this off on a specific big project?

We could then release a canonical package that includes a postcss transform or some such that could be used to decorate a CSS file with the same wrapper classes, providing the same basic benefit for the smaller subset of developers who want a build system.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 14, 2018

Contributor

One important advantage of making it runtime compared to a build tool is that we're not attached to a specific DOM structure, the transformations can be updated over time to match Gutenberg classes etc...

On the go, so apologies if I’ve missed this while skimming the code: but what about making it filterable, so a dev who wanted a build step could turn this off on a specific big project?

This is already possible, you just enqueue your CSS script like you'd do prior to this PR. The transformations should be rewritten using post-css in your build scripts.

We could then release a canonical package that includes a postcss transform or some such that could be used to decorate a CSS file with the same wrapper classes, providing the same basic benefit for the smaller subset of developers who want a build system.

The PostCSS transforms (not all of them) are available in the first commits, I think it's a great idea to showcase those but not certain if it should be "community" territory or "Core" especially because of the point about the backwards compatibility (DOM structure changes)

Contributor

youknowriad commented Sep 14, 2018

One important advantage of making it runtime compared to a build tool is that we're not attached to a specific DOM structure, the transformations can be updated over time to match Gutenberg classes etc...

On the go, so apologies if I’ve missed this while skimming the code: but what about making it filterable, so a dev who wanted a build step could turn this off on a specific big project?

This is already possible, you just enqueue your CSS script like you'd do prior to this PR. The transformations should be rewritten using post-css in your build scripts.

We could then release a canonical package that includes a postcss transform or some such that could be used to decorate a CSS file with the same wrapper classes, providing the same basic benefit for the smaller subset of developers who want a build system.

The PostCSS transforms (not all of them) are available in the first commits, I think it's a great idea to showcase those but not certain if it should be "community" territory or "Core" especially because of the point about the backwards compatibility (DOM structure changes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment