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

[Try] Remove inline style attributes paragraph and button. #3669

Closed

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Nov 27, 2017

Description

This PR removes inline style attributes from paragraph and button. The approach is generic and is able to remove inline style attributes from any block. It is a first step in the list proposed by @mtias in #2862. It is a work in progress and things are subject to change any inputs/thoughts are welcome.

As we are discussing in #2862 and #3090, this approach is used when setting very specific custom styles e.g a unique color not provided by theme/Gutenberg, parallel efforts will happen to allow themes and/or Gutenberg to provide readable human-friendly classes in font-sizes and common colors. Themes will also be allowed to remove the custom styles created.

The class names are generated using a short hash of the attributes. If we have the same styles used in many blocks only one class is generated. The output of the block is a function of its attributes including the classes. As the class is a function of its attributes if there is a set of attributes that when changing theme does not look nice, theme developers can overwrite that class changing all the blocks using this attributes. Although "hacky", this provides a simple mechanism to fix things when migrating designs.

Button and paragraph use different logic. In the paragraph, we generate one style class per custom paragraph e.g: "custom-paragraph-d6hya" so styles can be reused between blocks of the same type. In button, we generate a style class for the different styles needed: e.g: "background-color-red, background-color-y6j2, color-blue". This increases reusability as the same class can be used even in different block types but generates more classes. Blocks should do a tradeoff between this logic to maximize reusability. E.g: for colors, color classes may make sense because the same color may be used in different block type, for more specific styles, aggregating them in the same class may be better. Helper functions should be provided to allow reusable styles (as we are doing for colors).

Right now it is possible to customize paragraphs and buttons,
and apply custom styles. We can see the generated classes and the associated CSS, in the code view
. We can publish and see the styles working exactly as before when they were an inline style attribute. We use one style HTML tag in the post. We may, in the future, add logic in the server to remove our style element when showing the post and add an end-point that outputs a CSS file for a given post id. I like to save the CSS in the content because if we render the content we should see exactly the same that we were supposed to see before.

In order to implement custom styles, blocks need to add a function called saveStyles that receives the attributes (as the save) and returns the styles. Some simple helpers were created so generating styles is easier. It is possible to see the changes blocks need to make to remove inline style attributes in paragraph and button blocks. If we follow this approach, an equivalent save styles function needs to be created in PHP so server-side blocks can make use of a similar mechanism.

Right now styles are saved in the post in a "style" block, that is ignored when parsing and is computed when serializing. If we follow this approach this concept needs to be polished. I'm thinking about the concept a "computed block", or a "higher order block", a block that when saving receives the other blocks to generate the output. It depends on the attributes present in the other blocks. The style would be one of this blocks, as it depends on the customization attributes of the other blocks to generate the CSS. This concept may also provide other advanced ways of customization and extensibility. But maybe a better concept exists.

What I would like to discuss in this work in progress review is:

Is the saveStyles function in the blocks an acceptable interface or we should follow an alternative?
E.g: when save returns react components, we can recursively iterate on them, remove the inline style attribute and add classes. The classes can be generated from the react styles using renderToString+ parsing/processing. This approach is react specific and for now, we need to offer a generic one but it is food for thought maybe a generic one can be created based on this.

What do you think of the approach to generate CSS/class names? Is a simpler approach possible? Can we provide more human-friendly class names?

Do you predict a problem if this approach is widely used?

When polishing the concepts for the CSS computing mechanism, is the concept of a computed block (a block that receives other blocks) something we can pursue?
Or we should follow alternative approach: e.g: update our post grammar to return the styles so javascript can ignore them on load and PHP can do it's logic e.g remove them, append styles generated in PHP side, and/or use an end-point that generates a CSS file.

Are our helper functions to generate styles something useful for block creators and the effort when using/not using inline style attributes equivalent?

Any problem arose when executing the manual tests bellow?

How Has This Been Tested?

  • Add a button and a paragraph change content but not styles. Verify in the code view that no CSS is generated.
  • Add some styles to paragraph and button, verify classes are now applied and the CSS is now visible in the code view.
  • Repeat the same customization in another paragraph/button verify classes are reused and no duplicate CSS is created.
  • Use a named color, in the button e.g. #ff0000 and verify a readable class is used.
  • Publish the post with customizations applied and verify that the post looks as expected after publishing.

Screenshots

screen shot 2017-11-27 at 12 32 51

screen shot 2017-11-27 at 12 32 37

cc: @samikeijonen, @mike-schut, @geoconklin

@jorgefilipecosta jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Nov 27, 2017
@jorgefilipecosta jorgefilipecosta self-assigned this Nov 27, 2017
@jorgefilipecosta jorgefilipecosta changed the title Remove inline style attributes paragraph and button. [Try] Remove inline style attributes paragraph and button. Nov 27, 2017
return <p className={ className ? className : undefined }>{ content }</p>;
},

saveStyles( { attributes } ) {
Copy link
Member

@gziolo gziolo Nov 27, 2017

Choose a reason for hiding this comment

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

What if the user picks white as color and black as background-color for both button and paragraph? Shouldn't we share the same class names for both blocks? With the current implementation, we will create one combined class for all properties that paragraph has.

In other words, if it would work also to have:

<p class="color-white background-color-black" />

when user picks the colors specified for the theme.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can also use in the paragraph the same approach as in the button. So classes can be reused in different block types, but for now, I intentionally used different approaches so we can see what is possible. The plan for colors from the theme and default Gutenberg palette is to use a set of classes this will only be used for colors outside the ones in the palette :)

@gziolo
Copy link
Member

gziolo commented Nov 27, 2017

Style block is visible in the inserter:

screen shot 2017-11-27 at 14 28 14

I don't think it's expected in its current shape:

screen shot 2017-11-27 at 14 29 13

@jorgefilipecosta
Copy link
Member Author

Hi @gziolo,

Style block is visible in the inserter:

Thank you for your report. A simple change was performed so it is hidden and we don't accidentally add a style block. This style block, for now, is not polished it is a small "hack". We need to have a concept that allows this kind functionality to exist. I'm thinking about a computed block, with options that allow it to be automatically added to a post, allow multiple instances or not and allow display on inserter or not. These blocks would receive the other blocks on saving. But I'm tottaly open to suggestions.

@mtias
Copy link
Member

mtias commented Nov 27, 2017

@jorgefilipecosta I haven't read through all the approach, but my thinking was that the <style> element would be generated server side using the attributes for text color and background color (which incidentally, might be worth converting to attributes.style.backgroundColor and attributes.style.textColor instead of root attrs) and not serialized at all. It would just be rendered before a block, before the post, or in wp_head.

@aduth
Copy link
Member

aduth commented Nov 27, 2017

Coming in very uninitiated, but: If we're expecting common styles such as text or background color, could we not define these as block supports extensions? Why do we need to generate unique class names for these blocks; is not is-red-text-color sufficient to any block with red color applied? Would want to limit the generated class names to those present in the post, but perhaps this could be done as part of the extensibility behavior server-side inspecting blocks, their support, and defined attributes (expecting a common attribute e.g. textColor). Related: #2751

@jorgefilipecosta
Copy link
Member Author

Hi @aduth, @mtias thank you for sharing your thoughts.

@jorgefilipecosta I haven't read through all the approach, but my thinking was that the <style> element would be generated server side using the attributes for text color and background color (which incidentally, might be worth converting to attributes.style.backgroundColor and attributes.style.textColor instead of root attrs) and not serialized at all

That may also be an option and may be much simpler. But it would only be for color attributes, or we would also be able to use it for other styles? I think for colors it may be an option the server simply checks the presence of color attributes and adds the custom color classes when needed. The serialized content would already have the classes applied assuming the server will add them right?

Coming in very uninitiated, but: If we're expecting common styles such as text or background color, could we not define these as block supports extensions? Why do we need to generate unique class names for these blocks; is not is-red-text-color sufficient to any block with red color applied?

The idea is having classes set for the Gutenberg default palette colors, and for the colors provided by the themes. Themes will provide a class created by them that applies the color. This will only apply to totally custom colors e.g:#FB78C5 for which no custom class exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants