Allow build process to split css files for front end and backend. #1418

BE-Webdesign · 2017-06-24T02:36:46Z

Related to #963.

This PR splits up some of our CSS build process to have seperate files
generated for blocks and editor specific styles of blocks. Block
specific styles are now loaded both on the front-end and back end.
Styles that should appear on both are currently in a block.scss file.

Styles added:

wp-edit-blocks: blocks/build/edit-block.css
wp-blocks: blocks/build/style.css

Caveats:

It was suggested that the reverse be done, by using an edit.scss to hold
editor specific styles. This seemed pretty daunting a task to me and I
did not fully grasp what benefit that would bring over this approach
either.

** Testing Instructions **
build the files via npm run build or npm run dev

Verify that the gallery columns work both on the front end as well as
back end.

Verify that the drop cap style still works on the back end. It can't work yet on the front end because the styles are not carried over: #1417

youknowriad

Great work. I think we're really close. I suggest adding guidelines to the styles included in block.scss

Always add the block generated class as a root level
Maybe limit to the block library folder

youknowriad · 2017-06-24T12:39:51Z

blocks/editable/block.scss

@@ -0,0 +1,16 @@
+.drop-cap-true .blocks-editable__tinymce:not( :focus )  {


I was kind of expecting us to only provide block stylesheets in the frontend (block.scss only inside the library folder), but I may be wrong.

I think we should do:

Moving the drop cap style to the text block styles because it's an option specific to the text block.

.blocks-editable__tinymce we should drop this className from the styling because it's an "edit" className only.

I don't think we should style a global drop-cap-true className but more something like wp-block-text.is-drop-cap-enabled (We should probably always enforce the generated block className as a root of these styles shared with the frontend.

Thoughts?

Yeah, we can probably move this and remove the tinymce stuff. I though it was somewhat odd that the drop cap was in editable and not in text. I had to do an ack to find where the drop cap was. I think the idea behind it though is that there could be other fields that want a drop cap when using editable?

I am also not sure whether we want to change the styles around too much in this PR, and instead have this PR be focused more around the actual piping of styles in our webpack configuration. Then in subsequent PRs we can figure out where the styles should go. I know @jasmussen would have good ideas on how to break things up and configure them. If we start changing the styles now, we would have to change things on the text block as well, because currently the drop cap class names are not even added. We can still do it in this PR, but I would like to avoid merge conflicts 😄

Yeah, we can probably move this and remove the tinymce stuff. I though it was somewhat odd that the drop cap was in editable and not in text. I had to do an ack to find where the drop cap was. I think the idea behind it though is that there could be other fields that want a drop cap when using editable?

In the backend, if the dropcap CSS is applied to the editable, deleting characters behaves really weirdly. I can't recall exactly what went wrong, but we solved it by deciding that the dropcap is applied only in the backend when your cursor isn't actually on the editable block. We'd obv. want it there always on the frontend.

@youknowriad yes, that was the original plan, a has-drop-cap class added to p.

youknowriad · 2017-06-24T12:40:42Z

blocks/library/gallery/block.scss

@@ -0,0 +1,39 @@
+
+.blocks-gallery {


Aside and could be done separately, but we should probably drop this className in favour of the generated one wp-block-gallery

Yeah, that sounds like a good idea, same apprehension from the above comment applies here, as I am not sure how much we want to change the styles in this PR.

If we do this, it will break on the editor side of things. Going to keep as .blocks-gallery for now.

@youknowriad let's figure out how to get these classes on the edit side of things.

We have these on edit now, so let's try to switch the class.

Forgot about that. Thank you for the reminder. 👍

Looks like it is wp-block-gallery now.

jasmussen · 2017-06-24T15:36:28Z

Nice work. Works as expected. A few quick questions: we will be compiling all core blocks to have front-end styles that load in a single stylesheet on the frontend, correct? I.e. styles for dropcaps, galleries, cover images etc. those all show up in a single stylesheet?

Also, can we remove comments from the compiled stylesheets? Or is there at least a way to limit them? Right now we have some CSS comments for the responsive variables, that show up multiple times in the generated CSS for some reason.

Can you talk a little bit more about how to style a block so that it works in both the editor and the front-end? This will also help for the documentation we'll want to write. Take for example the simplest block, the separator. It currently only has the following CSS in the backend:

.blocks-separator {
	border: none;
	border-bottom: 2px solid $dark-gray-100;
	max-width: 100px;
	margin: 1em auto;
}

Aside from the fact that we may not want to style the HR at all (and let themes do that), let's imagine that we did want to style it, what would the style.scss then look like for the block? For example we'd probably always want to try and avoid assigning colors, and leave that totally to the theme.

Note, asking not as critique of this PR, I think it's probably good to go given Riads changes, just trying to wrap my head around it. Thanks for working on this!

youknowriad · 2017-06-24T15:41:59Z

Yes, the extracting mechanism and loading the style for frontend is good for me. As I said maybe I'd limit it to the "library" folder instead of all the blocks module.

I'm ok with merging and working on the stylesheets separately (we need guidelines on how separate edit and common styles effectively).

BE-Webdesign · 2017-06-24T15:53:39Z

Note, asking not as critique of this PR

Critique is welcome.

just trying to wrap my head around it

Yeah, same lol. Not totally sure how this should be done, but the approach I took will split out any css in our blocks directory into two separate css files. Any block.scss files will be built into blocks/build/style.css, which is registered in WordPress as wp-blocks. Then any of the remaining .scss files in the blocks directory will be built into blocks/build/edit-block.css registered as wp-edit-blocks in WP. The rest of the build process is untouched. This is just one way you can enqueue the block styles in the editor and on the front end of a theme.

Also, can we remove comments from the compiled stylesheets? Or is there at least a way to limit them? Right now we have some CSS comments for the responsive variables, that show up multiple times in the generated CSS for some reason.

Yeah, going to open a separate issue as this already happens on our styles and makes them a lot bigger than they should be.

Aside from the fact that we may not want to style the HR at all (and let themes do that), let's imagine that we did want to style it, what would the style.scss then look like for the block?

No idea, the HR styles seemed admin specific, but we can port them over. Maybe it is fine to have these be the default styles for the front end even with our colors. In #1420, people will be able to override the default styles in their theme, so maybe we can move a lot of stuff to be shown in the theme and expect people to override them. A lot of people seem confused at how Gutenberg does not match the front end, so it could be good to maybe go crazy and bring a lot of stuff to the front end to just see what happens and how people respond.

BE-Webdesign · 2017-06-24T15:54:54Z

Yes, the extracting mechanism and loading the style for frontend is good for me. As I said maybe I'd limit it to the "library" folder instead of all the blocks module.

Yup, I think I better understand what you were saying about the drop cap now. Instead of changing that stuff in the editable folder add the styles to the text block in the library?

nylen · 2017-06-26T13:02:34Z

webpack.config.js

+const BlocksCSSPlugin = new ExtractTextPlugin( {
+	filename: './blocks/build/style.css',
+} );
+


These vars should be lower-cased.

Sounds good.

nylen · 2017-06-26T13:02:49Z

webpack.config.js

+				exclude: [
+					/blocks/,
+				],
+				use: MainCSSExtractTextPlugin.extract( {


There is a lot of duplication here. Can we split this logic out into a function?

nylen · 2017-06-26T13:04:24Z

I think anything we do in this area should be designed to work with plugins that will need to register blocks + styles as well as with our own blocks. Otherwise, we are just going to do this work twice.

This would mean that we first build stylesheets for each "core" block separately, then in this PR or a separate one, register each block on the server along with its stylesheets.

youknowriad · 2017-06-26T13:12:59Z

@nylen People are asking for frontend support and I think it could be great to ship this in our weekly releases while we polish the API. (unless we could get the API quickly, but the consensus seems harder to find there)

nylen · 2017-06-26T13:28:46Z

I'm not saying that we need to ship a perfect and fully-formed API for this on the first try, just that we need to put some thought into our known use-cases before we ship this.

This would mean that we first build stylesheets for each "core" block separately, then in this PR or a separate one, register each block on the server along with its stylesheets.

Thinking about it some more, this is probably overkill for not much benefit, and registering a block stylesheet + an editor stylesheet per plugin is probably sufficient, as well as easier to build.

BE-Webdesign · 2017-06-26T18:11:37Z

Updated, waiting on GitHub to pull in changes. The update/split-editor-block-styles branch has the updated files.

youknowriad · 2017-06-26T21:03:45Z

blocks/library/text/index.js

+			}
+
+			return <p style={ { textAlign: align } }>{ content }</p>;
+		}


We could probably simplify the logic here to avoid duplication

const className = dropCap ? 'has-drop-cap' : undefined;

And always add the className prop. (undefined is equivalent to unset)

That would definitely be a lot better.

swissspidy · 2017-06-27T08:30:46Z

blocks/library/text/index.js

@@ -85,13 +86,14 @@ registerBlockType( 'core/text', {
 	},

 	save( { attributes } ) {
-		const { align, content } = attributes;
+		const { align, content, dropCap } = attributes;
+		const className = dropCap ? 'has-drop-cap' : undefined;


This could simply be const className = dropCap && 'has-drop-cap';

Sounds good.

mtias · 2017-06-28T11:21:28Z

blocks/library/text/block.scss

@@ -0,0 +1,16 @@
+.has-drop-cap {


We should write this as p.has-drop-cap, since modifier classes should never be used without a main class or element as a target.

Okay, sounds good.

BE-Webdesign · 2017-06-28T21:37:01Z

Rebased, resolved conflicts, and made the latest recommended changes. Let me know if anything else needs a touch up.

Related to #963. This PR splits up some of our CSS build process to have seperate files generated for blocks and editor specific styles of blocks. Block specific styles are now loaded both on the front-end and back end. Styles that should appear on both are currently in a block.scss file. Styles added: wp-edit-blocks: blocks/build/edit-block.css wp-blocks: blocks/build/style.css Caveats: It was suggested that the reverse be done, by using an edit.scss to hold editor specific styles. This seemed pretty daunting a task to me and I did not fully grasp what benefit that would bring over this approach either. ** Testing Instructions ** build the files via `npm run build` or `npm run dev` Verify that the gallery columns work both on the front end as well as back end. Verify that the drop cap style still works on the back end.

Adding documentation about the build process, updating variable names, reducing duplication. Making the drop cap style work on the front end as well as gallery styles.

Fixing test to not include empty class property.

Changing a ternary operator into an && statement. Adding a p.has-drop-cap as the selector instead of just .has-drop-cap.

youknowriad · 2017-06-29T15:09:10Z

Merging this, Let's get this in tomorrow's release. I'm going to work on splitting the other blocks' styles.

BE-Webdesign · 2017-06-29T15:13:02Z

Should we restart Travis? Tests passed locally. Not sure why the build stalled.

aduth · 2017-07-03T15:28:30Z

Any block.scss files will be built into blocks/build/style.css, which is registered in WordPress as wp-blocks. Then any of the remaining .scss files in the blocks directory will be built into blocks/build/edit-block.css registered as wp-edit-blocks in WP.

So in practice, block.scss is the "base" styles for a block we expect to apply in all contexts, and style.scss are editor-specific additions? To me, the naming could do for better distinction. edit.scss (or editor.scss) was mentioned in caveats of original message. This seems more sensible to me to at least provide a hint to the difference between the two stylesheets. Right now it's quite unclear.

BE-Webdesign · 2017-07-03T16:43:16Z

@aduth

I agree, the problem I encountered while working on this is that most of the style.scss styles are editor related styles, and we have multiple other style sheet names, which contain mostly editor related styles. So as good as the naming for edit.scss etc. is, the practicality of making those changes in one PR would be merge conflict prone. This PR on its own had merge conflicts, can't imagine what it would have been if everything was renamed and relocated at once. The way things are set up things, styles can be slowly migrated into edit.scss and block.scss.

BE-Webdesign mentioned this pull request Jun 24, 2017

Front end styling for blocks. #1217

Closed

youknowriad reviewed Jun 24, 2017

View reviewed changes

nylen reviewed Jun 26, 2017

View reviewed changes

BE-Webdesign force-pushed the update/split-editor-block-styles branch from 19e54d5 to a55a004 Compare June 26, 2017 18:18

youknowriad reviewed Jun 26, 2017

View reviewed changes

BE-Webdesign force-pushed the update/split-editor-block-styles branch from c96b06b to d3cc431 Compare June 27, 2017 02:46

youknowriad mentioned this pull request Jun 27, 2017

Fix undefined drop-cap class #1468

Merged

swissspidy reviewed Jun 27, 2017

View reviewed changes

mtias reviewed Jun 28, 2017

View reviewed changes

BE-Webdesign force-pushed the update/split-editor-block-styles branch from d3cc431 to cb68ea6 Compare June 28, 2017 21:36

BE-Webdesign added 5 commits June 29, 2017 16:03

Changes related to feedback.

82e6d90

Adding documentation about the build process, updating variable names, reducing duplication. Making the drop cap style work on the front end as well as gallery styles.

Fix Test

34079b2

Fixing test to not include empty class property.

Clean up text block save method.

68c9c57

Updating with minor fixes

4b5f5fa

Changing a ternary operator into an && statement. Adding a p.has-drop-cap as the selector instead of just .has-drop-cap.

youknowriad force-pushed the update/split-editor-block-styles branch from cb68ea6 to 4b5f5fa Compare June 29, 2017 15:03

youknowriad approved these changes Jun 29, 2017

View reviewed changes

youknowriad merged commit 8dfc4c5 into master Jun 29, 2017

youknowriad deleted the update/split-editor-block-styles branch June 29, 2017 15:41

youknowriad mentioned this pull request Jun 29, 2017

Blocks: Split frontend block styles into block.scss files #1590

Merged

nylen mentioned this pull request Jul 5, 2017

Add actions for plugins to register frontend and editor assets #1717

Merged

danielbachhuber mentioned this pull request Apr 14, 2018

Fix Issue #6105 regarding drop cap-related display error #6133

Closed

3 tasks

ellatrix mentioned this pull request Apr 23, 2018

Restore hiding drop cap on focus to prevent bugs with contenteditable #6359

Merged

4 tasks

		@@ -0,0 +1,16 @@
		.drop-cap-true .blocks-editable__tinymce:not( :focus ) {

Allow build process to split css files for front end and backend. #1418

Allow build process to split css files for front end and backend. #1418

Conversation

BE-Webdesign commented Jun 24, 2017

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BE-Webdesign Jun 24, 2017 • edited Loading

Choose a reason for hiding this comment

BE-Webdesign Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Jun 24, 2017

youknowriad commented Jun 24, 2017

BE-Webdesign commented Jun 24, 2017

BE-Webdesign commented Jun 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Jun 26, 2017

youknowriad commented Jun 26, 2017

nylen commented Jun 26, 2017

BE-Webdesign commented Jun 26, 2017

youknowriad Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BE-Webdesign commented Jun 28, 2017

youknowriad commented Jun 29, 2017

BE-Webdesign commented Jun 29, 2017

aduth commented Jul 3, 2017

BE-Webdesign commented Jul 3, 2017 • edited Loading

BE-Webdesign Jun 24, 2017 •

edited

Loading

BE-Webdesign Jun 26, 2017 •

edited

Loading

youknowriad Jun 26, 2017 •

edited

Loading

BE-Webdesign commented Jul 3, 2017 •

edited

Loading