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

Provide the active style name as a prop to edit/save functions of the blocks. #13506

Open
JoshuaDoshua opened this issue Jan 25, 2019 · 7 comments
Labels
[Feature] Block Variations Block variations, including introducing new variations and variations as a feature [Type] Enhancement A suggestion for improvement.

Comments

@JoshuaDoshua
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Currently, blockStyles add is-style-STYLENAME to both className and to attributes.className. If we are setting a "style" of a block, we should be able to access the raw style name. The attributes.className is just passed included in the props.className calculation. This makes attributes.className is a shortcut to add additional classes to props.className but blockStyle will attempt to override that.

Also, this makes authoring styles for the block more difficult if you can only go off of the parent is-style- class. We don't use any is-style- declarations, though we do believe that we should maintain the default Gutenberg className at the wp-block- level for consistency.

Describe the solution you'd like

  1. Simply adding blockStyle: STYLENAME (without "is-style-") to the attributes would make this much clearer, and give developers more control over the inner html of the block.

  2. Additionally, the className calculation could be altered to check for a blockStyle property and add it there. This would free up the className attribute for uses outside of blockStyles, keep the is-style-STYLENAME in props.className, and prevent duplicate attributes being saved in the database.

Since we are adding more freedom and not removing/altering any of the actual output, I don't think this would break any backwards compatibility.

Describe alternatives you've considered
To get around this, we could just remove the is-style- from the className and use it in js, but this feels hacky and doesn't allow us freedom of the attributes.className property.

tldr; We want to be able to say .block__inner-element--STYLENAME not .wp-blocks-block.is-style-STYLENAME .block__inner-element and allow STYLENAME to be more accessible from javascript.

@JoshuaDoshua
Copy link
Contributor Author

example of current way to get style name

const isStyle = RegExp(/is-style-/)
const styleName = isStyle.test(attributes.className)
    ? attributes.className.replace(isStyle, '')
    : null

@youknowriad
Copy link
Contributor

Seems reasonable to expose a custom prop in the edit/save function providing the active style variation.

That said, I think we should stick with the current className. Using className allows for style variations to be registered/unregistered without invalidating blocks and the style variations are modifier classNames, it makes sense to keep the current syntax.

@youknowriad youknowriad added [Type] Enhancement A suggestion for improvement. [Feature] Style Variations Related to style variations provided by block themes labels Jan 25, 2019
@youknowriad youknowriad changed the title blockStyle value should be implemented outside of className Provide the active style name as a prop to edit/save functions of the blocks. Jan 25, 2019
@JoshuaDoshua
Copy link
Contributor Author

@youknowriad good point I didn't think about register/unregistering styles, but I'm not sure I follow (and definitely not as familiar with the source code as you are). Apologies, if I was unclear; I'm definitely not proposing removing the .is-style- modifier-class.
I was under the impression that block validation looked for the HTML from the save function based on the console validation errors. So even with any unregistered styles, the existing props should remain the same for the block unless the user selected a new blockStyle no?
The BlockStyles component adds the className to both the block.props and blocks.props.attributes right? So updating that component to change the saved attribute name/value to be more concise and usable wouldn't affect the modifier-class.

The only problem I saw with unregistering previously selected styles is that the incorrect style is selected in the Inspector, but this could be resolved relatively easily I would think and I'm not sure that's directly related to this proposal.

// current
props: {
    className: 'wp-blocks-example is-style-primary',
    attributes: {
        className: 'is-style-primary',
        ....
    }
}
// <div class="wp-block-example is-style-primary">....
// proposed
props: {
    className: 'wp-blocks-example is-style-primary',
    attributes: {
        activeStyle: 'primary',
        ....
    }
}
// <div class="wp-block-example is-style-primary">....

Unless props.className is coupled to props.attributes.className somewhere that I'm not seeing. Then just adding the activeStyle attribute would be helpful.

I'm planning on opening a PR and testing these out with existing blocks/styles, just wanted some core contributor feedback beforehand.

@youknowriad
Copy link
Contributor

I don't think it should be an attribute though, because it's persisted as the className attribute and applied automatically using the hooks taking care of the custom classNames.

And the custom className hook has a specific behavior, it's the only attribute that don't invalidate the block if we find a difference between the attribute value in the comment and the attribute value in the HTML. Using another attribute will break this. Not to mention that using another attribute could be considered a breaking change which is not allowed in WordPress at this stage unless we figure a deprecation strategy for WordPress Core. There's a discussion about this currently but that's out of scope of this issue.

@JoshuaDoshua
Copy link
Contributor Author

that makes sense, thanks @youknowriad ; I'll get to working on a PR

@mikeselander
Copy link
Contributor

I am strongly in favor of adding a prop passed into a component with a raw style name. Using the style as a className restricts usage of the style attributes and excludes several reasonable usage of styles.

As an example, this week I found myself building a block with several styles that use legacy CSS classes from an existing theme. Re-factoring or extending those classes into is-style- format is untenable because we're porting shortcodes into blocks and the shortcodes and blocks will both be used on the website for quite some time. Breaking these classes into simple is-style- types would cause duplication/maintenance issues and unknown quantities of regressions. I therefore had to extract the raw style name out of the classname using similar regex as that above on both the client and server-side.

Styles is an excellent UI for presenting visual differences in blocks but is far too tightly coupled to a CSS class. It expects a static block, a brand-new clean slate of styles, and a WordPress way of styling things that doesn't exist on most real-world sites.

By adding an attribute or prop, developers could more easily manipulate inner blocks, child element CSS classes, present different attribute options, and so much more. In the reverse, expecting developers to do the regex mentioned by @JoshuaDoshua on the client (and potentially also server) is a poor developer experience.

@youknowriad I can't say I understand why adding a new attribute would be breaking (I would like to know more about that), but perhaps passing a prop into the component similar to clientId could alleviate that concern? This would be an additional prop to be used as-needed by developers and core bocks could continue to use the className attribute to handle their style variations.

@strarsis
Copy link
Contributor

strarsis commented Aug 21, 2020

@JoshuaDoshua: Your function sadly won't work at all when there are extra classes than just the is-style-... class (like an alignment-specific class (vertical alignment))...

This function works with multiple classes:

const getActiveStyle = function(className) {
  const classes = className.split(' ');
  const isStyle = RegExp(/is-style-/);
  for(let classIndex in classes) {
    let classStr = classes[classIndex];
    if(isStyle.test(classStr))
      return classStr.replace(isStyle, ''); // return and stop at first occurence
  }
  return null;
}
export default getActiveStyle;

@colorful-tones colorful-tones added [Feature] Block Variations Block variations, including introducing new variations and variations as a feature and removed [Feature] Style Variations Related to style variations provided by block themes labels Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Variations Block variations, including introducing new variations and variations as a feature [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

5 participants