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

Added support for custom CSS properties in the StyleWrapper #5581

Merged
merged 13 commits into from Jan 2, 2024

Conversation

sneridagh
Copy link
Member

@sneridagh sneridagh commented Dec 27, 2023

This is a pattern I use lately, and it makes a lot of sense to me.

A couple of questions:

  • Would it be useful/needed to also inject a className in order to hook to? I was about to do it, but I don't know if that would be required... maybe to know if a value is set or not... but you can always have a default fallback value.
  • Naming... what not! I chose CSSProperty as the sufix... any other suggestions?

I'm well aware that this opens the door to fine grained style customizations, if we extend the pattern not only to use custom CSS properties but also use random CSS properties (another converter should be created, then do not append the -- and disable nested support, then) I leave it up to the community. However, the initial feeling when we came up with the style wrapper stands: I wouldn't like the wrapper to be missunderstood, and that people are able to easy create blocks that resemble WordPress Elementor...

Let's iterate over this, in the meanwhile, I will work on integrate the PoC into VLT.

Update 01/01/2024: Answer to questions:

  • It's not really needed, since one could build a selector like this:
 .block.teaser[style*='--background-color'] {
    padding: 20px 0;
}

if required.

  • This PR is not using a suffix discriminator to differentiate from the others. Now it detects if the property starts by -- see the documentation.

Copy link

netlify bot commented Dec 27, 2023

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 7f449db
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/6592e6f87d986600083203d7

Copy link

netlify bot commented Dec 27, 2023

Deploy Preview for plone-components ready!

Name Link
🔨 Latest commit 7f449db
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/6592e6f8246b720008557d02
😎 Deploy Preview https://deploy-preview-5581--plone-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Although not part of this PR, I found classname and fieldname and their plural forms to be weird. Are they supposed to be inline literals? If yes, then they need to be wrapped with single backticks and use the correct casing, else they should be "class name" and "field name".

docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
packages/volto/news/5581.feature Outdated Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
sneridagh and others added 2 commits December 28, 2023 08:59
Committed what makes sense for now.

Co-authored-by: Steve Piercy <web@stevepiercy.com>
@stevepiercy stevepiercy self-requested a review December 28, 2023 09:30
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Fixed a heading and follow up comments.

docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
sneridagh and others added 2 commits December 28, 2023 11:04
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Thank you!

docs/source/blocks/block-style-wrapper.md Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Show resolved Hide resolved
@tiberiuichim
Copy link
Contributor

The slope is steep and full of grease. Pretty soon we'll end up here: https://raw.githubusercontent.com/eea/volto-block-style/master/docs/screenshot.png

When I started volto-block-style I had the same intention: let me create a way to centralize block "themes" and make it possible to assign those. That's why the "Style presets" widget is placed first in that volto-block-style sidebar. Pretty soon it devolved into whatever this is: https://github.com/eea/volto-block-style/blob/1fd970881ca1d816f8db764cdb2498348f10f191/src/StyleWrapper/schema.js#L3

@sneridagh
Copy link
Member Author

@tiberiuichim I know... it's not easy. It's very hard to predict which techniques and patterns are the best for the use case.

When I first pushed for the wrapper I had one thing clear: The intention was not to provide an easy way to plague Volto blocks settings with Elementor-like controls... because it's not what we are aiming for in Volto. volto-style-block was a bit of that. It's absolutely legit if you have a client that explicitly asks for this, and it's editors are real power users who knows how the web works and are capable of handling this complexity. So not enabling doing it in an easy way was important for me.

This is something in between, still with a clear purpose, and documented so people can know how to take advantage of it. A width value, a color, so you can then take it in the CSS and use it right away, one shoot only CSS.

As I said, this opens the pandora's box by perverting it could be used to really inject arbitrary CSS properties. If you really wanted to do it, you can, but on your own. Although, given the power of using custom CSS properties instead, who wants this nowadays?

Copy link
Sponsor Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

Thanks for changing it so the developer can just write the property name the same way it is rendered; I think that makes it easier to use.

I do have some of the same doubts about whether people will use the feature judiciously, but the potential problem of adding too many options is there whether or not this feature exists. Would it help to give an example in the docs of a situation where using this would be appropriate and one where it would not?

@sneridagh
Copy link
Member Author

@tiberiuichim @davisagli I am testing it in a current project, and I have to say that the result surprised me quite a bit. I didn't expect it to be so convenient.

Let me explain, Volto Light Theme as starting point. We have in there in place a global CSS custom property --image-aspect-ratio so all the images in blocks (teasers, listings too) use it. The idea was that someone in his customized theme could set it and change them all in runtime, because, custom CSS properties.

When the feature in this PR is used combined with the above you can enhance a block's schema (eg. teaser) as follows:

  schema.properties.styles.schema.fieldsets[0].fields = [
    ...schema.properties.styles.schema.fieldsets[0].fields,
    '--image-aspect-ratio',
  ];

  schema.properties.styles.schema.properties['--image-aspect-ratio'] = {
    widget: 'select',
    title: 'Aspect Ratio',
    choices: [
      ['1', '1:1'],
      ['16 / 9', '16/9'],
    ],
    default: '1',
  };

Then, the markup of the block will contain the custom property:

<div class="block teaser" style="--image-aspect-ratio: 1">
...
</div>

So, locally, that block and only that block, will apply that CSS definition without having to change a single line of code of CSS.

I will add this to the docs.

@sneridagh
Copy link
Member Author

@davisagli I improved the example and use case in the documentation. I thought about a "bad example" given the current implementation, and I can't come up with one. I guess that one would still to come up with a mapping 1:1 of custom properties and basic arbitrary CSS properties in CSS or completely override and allow arbitrary CSS properties injection. What about documenting the reasoning behind we do not allow them?

@stevepiercy please take a look again!

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

I am unsure of a couple of passages, and noted them as such.

I also suggested reordering the code blocks and their narrative text to improve flow.

docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
I think it flows better to define the CSS property first, then show how to inject it with JS, and finally how it appears in HTML.
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

I think we're getting closer. I rearranged the CSS to be first, so it flows better, and filled in a couple of pieces I thought were missing. Finally, I took another stab at those two sentences that confuse me more than help me understand.

docs/source/blocks/block-style-wrapper.md Outdated Show resolved Hide resolved
@sneridagh sneridagh merged commit 018d58b into main Jan 2, 2024
64 checks passed
@sneridagh sneridagh deleted the supportForCustomCSSPropertiesInStyleWrapper branch January 2, 2024 09:34
sneridagh added a commit that referenced this pull request Jan 2, 2024
Co-authored-by: Steve Piercy <web@stevepiercy.com>
@stevepiercy
Copy link
Collaborator

These two sentences got missed again in the review. I'll create a separate PR to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants