Core Blocks Style Wrapper

sneridagh commented 2 years ago

Initial implementation, subject to change.

Please, feedback is needed!

netlify[bot] commented 2 years ago

Deploy Preview for volto ready!

Name	Link
Latest commit	b421ef078bb1545524f7132f83a8ed056ad12033
Latest deploy log	https://app.netlify.com/sites/volto/deploys/6278df6b0a26a100090aade9
Deploy Preview	https://deploy-preview-3269--volto.netlify.app/blocks/block-style-wrapper.html
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

tiberiuichim commented 2 years ago

This seems nifty (from the description). I can't really run the product, but the blurb looks promising: https://github.com/components-ai/css.gui

pnicolli commented 2 years ago

Nice addition. It this planned for Volto 16? Imho as of today this is a potential breaking change, it changes the dom structure of pages.

giuliaghisini commented 2 years ago

I'm not very keen on adding a wrapper to existing blocks. I think it is better to add the class to the existing block wrappers, for example <div class = "block image bg-gray full-width"> ... </div> or use the wrapper inside the block, for example:

<div class = "block image"> 
      <div class = "bg-gray full-width> </div> 
</div>

Even the forced use of the Container does not drive me crazy. There are several situations where you don't want to use the Container at that point (for example if in a full-width block I want to use an image as a background, etc.)

In RedTurtle we have already developed something similar that could be inspired by, and we have also implemented a widget for choosing colors from a predefined list. It would be nice to also introduce this widget together with the ColorPicker, in order to give two possible solutions to the developers based on the freedom they want to give to the editors.

We could take a look during the sprint. For example this is what we do:

https://user-images.githubusercontent.com/51911425/165303778-297093d6-ea15-4970-946a-73479b44da75.mov

sneridagh commented 2 years ago

@plone/volto-team

We should discuss this matters during the sprint:

Force the inner Container in the wrapper (as @pnicolli hinted) might be not required, but only if we change how we do things in the default layout. It would imply to remove the main Container in the #page-document, #page-edit, #page-add (main) block containers, so we control the flow in the block itself. BUT, that is a breaking.

So do we want to issue that kind of breaking change just before Plone 6 is shipped? Is it worth it? Add-ons and themes would have to adjust. Granted, it won't be a big one, but still...

Should this include then a full re-work of the layout in core already? We were pushing for this kind of layout in our current and latests projects:

Frame 10

It follows the latest trends in websites, and it's quite appealing for our clients.

sneridagh commented 2 years ago

We have to list all the use cases that we want to address then find if we can spare the container (I think we do still have it for some reason).

sneridagh commented 2 years ago

@stevepiercy thanks for reviewing! I'll take a look at the rest of the things, the docs are still in flux and still need more love.

giuliaghisini commented 2 years ago

@plone/volto-team

We should discuss this matters during the sprint:

Force the inner Container in the wrapper (as @pnicolli hinted) might be not required, but only if we change how we do things in the default layout. It would imply to remove the main Container in the #page-document, #page-edit, #page-add (main) block containers, so we control the flow in the block itself. BUT, that is a breaking.

So do we want to issue that kind of breaking change just before Plone 6 is shipped? Is it worth it? Add-ons and themes would have to adjust. Granted, it won't be a big one, but still...

Should this include then a full re-work of the layout in core already? We were pushing for this kind of layout in our current and latests projects:

It follows the latest trends in websites, and it's quite appealing for our clients.

i think this will be a breaking change that needs to review all the previeous project when upgrading volto version.. Maybe, it will be an option or a template for the block 'Title' (that could become a combination of title and leadimage block', what do you think?

tiberiuichim commented 2 years ago

I'm +1 to introduce the new layout for the Plone 6 release, to be worked as part of the Bethoven sprint. It is a major step forward to making Volto more flexible and easier to understand and control.

If needed, we can think about methods to keep backwards compatibility. This problem can be solved in the layout view component and the block style wrapper.

sneridagh commented 2 years ago

@giuliaghisini oh, no. Sorry I did not express myself correctly. Ignore the first block, just take a look at the overall block widths and their disposition, depending on if they are floating or not. Also, note the "default" text block width is narrower, so we have this full/normal/narrow container types for blocks.

sneridagh commented 2 years ago

Nice addition. It this planned for Volto 16? Imho as of today this is a potential breaking change, it changes the dom structure of pages.

Not anymore!

sneridagh commented 2 years ago

@stevepiercy I amended a bit the docs, if you want to have a final look.

sneridagh commented 2 years ago

@plone/volto-team I refactored it today, following all the feedback received, thanks a lot! I think this is generic enough, and non breaking to merge it.

However, I'd wait since @giuliaghisini told me about the awesome RT style/color picker. Is it open sourced yet?

Also, we can work on the layout once this is in place.

If we merge it asap, let's issue a 16 alpha right away and let's maintain it during the sprint and the aftermath.

sneridagh commented 2 years ago

Also: tempted to add the 'block blockType' automatically too, since we do this all the time. Opinions?

ksuess commented 2 years ago

I do not understand the pros of this concept.

An editor selects a background color 'orange'. The block gets the class 'has--backgroundColor--orange' Only if the developer provides a CSS rule for this class, something is styled. As soon as the editor changes to another color. There is no styling as long as the developer does provide a CSS rule for the new color.

Another topic: presets. I'm using Eau de Webs block styler with presets: three sets of CSS rules (background color, padding, font color) That's to offer the editor just three options and not a full bunch of colors to ruin the layout. edw block styler adds a class per selected preset. That's making live easy for the editor (he does not have to be creative, he selects from three options), also for the developer (if the client wants highlighted blocks green instead of blue: it's just a change in config.js.

tiberiuichim commented 2 years ago

@ksuess One advantage of using explicit classes for colors is that they're "portable". If the website look changes, then you can adjust the class styling. The eeacms/volto-block-style is too blunt in its approach.

@sneridagh Maybe if we have the CSS props included, it would make more sense on why this approach was chosen?

ksuess commented 2 years ago

@ksuess One advantage of using explicit classes for colors is that they're "portable". If the website look changes, then you can adjust the class styling. The eeacms/volto-block-style is too blunt in its approach.

Yes, I see the abstraction layer. That's why I like to offer just a set of options to the edior.

sneridagh commented 2 years ago

@ksuess the color chooser is still in progress. It should be configurable, per block to allow you only add the presets per project for that block.

I'll continue working on this during the next days, for now, I'd like to know what do you think about the flexibility that the foundations of this has. The idea is to have presets in core as well, so we have a set of well known set of styles (extensible) by default.

Do you think that it's valuable to set the CSS directly in the style? I think that mapping values in the settings directly to real CSS definitions is very limited, and will end in a dozen of properties in there, that only power users know their purpose. I think that map that to abstract styling is better, so you can (potentially) have control over other properties, depending on the preset. That's why I think that the style preset is better over mapping CSS properties alone.

Of course, the wrapper allows you to map them in that way, if you feel that matches also your requirements. Using classes, makes them portable too (and configurable)... using custom CSS properties, like:

@headerFont: var(--custom-headers-font, @pageFont);

Then customise per project the CSS prop, without touching the CSS class definitions.

ksuess commented 2 years ago

I assume that everyone wants both or at least the first:

presets per project (and additional to edw's styler: per block type)
individual fine granular options for the editor

I personally try to convince the customer to offer only the first.

Your increased configurability with a schema enhancement is of course welcome. If understand right it would look like

  config.blocks = {
    ...config.blocks,
    blocksConfig: {
      ...config.blocks.blocksConfig,

      slate: {
        // (more block settings)
        ...config.blocks.blocksConfig.slate,
        stylesEnabled: true,
        stylesSchema: myCustomStyleSchema,
        availableColors: ['orange', 'green'],
        availableFonts: ['foo', 'bar'],
        availableSomething: ['trallalala']
      },

depending on the custom schema 'myCustomStyleSchema'.

I'm just puzzled how the CSS rules can be written for this big bunch of classes that are generated by the individual options (2. case above) for

<div className="has--backgroundColor--ee22ee has--myCustomStyleField--red has--myCustom2StyleField--color--black has--myCustom2StyleField--color--MyGradient">

But I understand that this abstraction is need to achieve an abstraction that allows to change the mapping later without fiddling with explicit CSS rules in blog data.

ichim-david commented 1 year ago

I'll continue working on this during the next days, for now, I'd like to know what do you think about the flexibility that the foundations of this has. The idea is to have presets in core as well, so we have a set of well known set of styles (extensible) by default.

Do you think that it's valuable to set the CSS directly in the style? I think that mapping values in the settings directly to real CSS definitions is very limited, and will end in a dozen of properties in there, that only power users know their purpose. I think that map that to abstract styling is better, so you can (potentially) have control over other properties, depending on the preset. That's why I think that the style preset is better over mapping CSS properties alone.

@sneridagh I am aware of what volto-block-style does with style tags. One advantage that it has over the class approach is the specificity gains from inline styles vs potentially a single class styled which get's overridden by more specific styles. Although this is a nice thing to have, you could potentially have changes to the class style if the styles were added in a class.

I don't think the current mapping approach is a good idea because it sets up the class name for failure anytime there is a change in the actual color value. "has--backgroundColor--AABBCC" If you decide to tweak the color slightly that class will then have a false naming. To me it seems Victor that you want to create a sort of object to CSS generation but instead of having a class created with all of the properties you have several classes named as inline style plus values, at least from the tests and current demo. If we go in the abstraction domain where you don't add an inline style but you get a class that you need to add your style then it would be better to name them different from the inline style values.

If the idea is to move the inline styles to classes then I think that on yarn build there should be a step that goes through the config, reads these style objects and appends these classes to a CSS file with a configurable location.

This way if you have const styles = { color: 'red', backgroundColor: '#AABBCC', }; and you output has-color-red and has-backgroundColor-AABBCC You don't need to add by hand these classes as they are provided for you when you build the site. When you have a change to the styles and you rebuild the file is reconstructed. This will fail though if the value changes as you will be left with orphan classes on the previously set blocks?

sneridagh commented 1 year ago

@ichim-david

I apologise for not having time to develop and properly write down the reasoning behind this approach before asking your feedback. I'll try to do it now, and continue the discussion during the sprint, because this always was the original idea.

Top-Down styling approach vs editor's per block instance free will

One of the selling points in Volto and the idea behind its UX was always to lead the user/editor to a easy, safe path, that will result into styling consistency and unified look and feel across the site. This is true for everything UX related in Volto, eg. we do not simply put in front of the editor a myriad of choices and let them the free will to combine them to reach to a random result each an every single time the user configures a block.

We could have a system that allow user to choose between a number of styling properties mapped directly to CSS inline styles. Elementor does that for WordPress... and IMHO the result from the user point of view is a mess. The average editor does not have a clue what a padding is, or how it works and relates to the items around, just to start. Then, the styles that you apply in one block are hardly reproducible in others, after days, weeks, which style did you set in the other section sliders?

I know that this could sound useful for power users, and one could say: hey, if the client wants just this, let's give it to them. That's fine, but it's not Volto's USP. That's probably why volto-style-block is how it is, so I wanted to give the option for the foundation of this to allow it, that's why the shape of the styles fields is how it is, so you can enhance it later and bend it to your needs.

I think that we should stick to the Top-Down approach for Volto, where you have a set of variations over a block, and you get that from your site's designer. We must sell that as a Volto's feature. And yes, force our clients to think into this direction and make it one of our selling points.

The idea is that the user/editor, in fact, is not going to (or rather, should not to) have this free will. So the choices will be given, and the model behind it only can be set with classes. More on this below.

Enter the block presets

So, ideally, you get a list and a mockup of all the blocks that the client is going to need in their future site, along with their variants. Let's call this presets. (We can find a better wording, doh naming!)

These are the grid block (with teaser) variations (or presets) of our last project:

These are the grid block (with text) variations:

You can see the slight differences that should be applied in elements inside (fonts, images, links, shadows, hovers) that can't be expressed with inline styles over a parent element. There are also some variants that need different font colors (only some of them, so you can't generalise), because of contrast with the background.

The styles object and classname generation also helps with modelling a set of classes for that. The first nesting level? For namespacing the presets. Maybe I overengineered this, and we don't need such an abstraction, but... Also, you can put the color name on the values of the keys, but I would not do that... I'd better name presets and variants. Time will tell if we should improve or refactor the idea.

For that, we need a widget so you can choose the presets and it's enough powerful to showcase them properly (doesn't have to be precise, only model them in a way that a user can easily map the preset to the actual resultant block variant. I liked a lot the RT chooser, but we can further improve it to match the needs.

Default block presets

One could say, that depending of the size of the project you won't have the luxury of such a detailed mockup set. I'd propose to have a default good known presets for that, that are easy overridable. Problem #1 is that we don't have a mockup of them, neither a set of colors that PastanagaUI defined back in the day. I can ping Albert, or rather, we find a set of colors that could work by default.

These defaults could be generic enough (using abstract naming) and driven by CSS properties, so one could customise only the CSS property on the project side, without any other amendment.

Also, maintain them may become daunting at some point. So I would initially not do so.

Where to go from here

As said, the idea is to set the foundation for something bigger and flexible. I'm ok to enable the inline styling so all possible cases are covered. I envisioned the styles object because of that as well.

By any means I would go the CSS in JS rabbit hole, but basic inline styling should be also possible as it is in volto-style-block, but not by default or used by vanilla Volto. We don't want another Elementor.

Any after-build option is not an option, let's keep this simple and transparent. The fact that you have to provide your CSS as preset is unavoidable, and a good thing.

cypress[bot] commented 1 year ago

Test summary

246 • 0 • 10 • 0

Run details


Project	Volto
Status	Passed
Commit	b421ef078b
Started	May 9, 2022 9:35 AM
Ended	May 9, 2022 9:47 AM
Duration	12:15 💡
OS	Linux Ubuntu - 20.04
Browser	Chrome 101

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

sneridagh commented 1 year ago

@plone/volto-team I think this is ready now. It sets the foundation for the future of block styling, and new layout. I'd say let's merge it since it's not a breaking change, let's start build upon it and get as much feedback as possible from testing it.

sneridagh commented 1 year ago

@stevepiercy I added some paragraphs more to the docs :P

sneridagh commented 1 year ago

@ichim-david waiting for your last ack!

plone / volto