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

Blocks: Automatically generate a wrapper classname for blocks #1381

Merged
merged 7 commits into from
Jun 23, 2017

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jun 22, 2017

try to close #1362

Glitches:

  • Doesn't work for blocks with save function returning strings (like embed links)
  • Doesn't work for block with save function returning a component (instead of an element) but IMO we should drop this because we don't need a Component in the save function (we don't need any local state, ref or something it's just a serialization)

Notes:

We may need the same thing on the backend for dynamically rendered blocks but it's harder to implement because we only have strings (and not element) in the backend.

@youknowriad youknowriad self-assigned this Jun 22, 2017
if ( ! element || ! isObject( element ) ) {
return element;
}
const className = classnames( element.props.className, `wp-block-${ kebabCase( blockName ) }` );
Copy link
Member

Choose a reason for hiding this comment

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

Can we drop core from the classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about plugin blocks with other namespaces? this could create ambiguity.

Copy link
Member

Choose a reason for hiding this comment

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

How so? Core blocks would have no namespace and plugin blocks will have the plugin namespace.

@@ -37,8 +39,18 @@ export function getSaveContent( save, attributes ) {
}
}

// Adding a generic classname
const addClassnameToElement = ( element ) => {
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to have a list of blocks where we don't want to apply the class (p and heading, to start).

Copy link
Contributor Author

@youknowriad youknowriad Jun 22, 2017

Choose a reason for hiding this comment

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

what about a block setting className that can be nullable and if not set we default to wp-block-core-quote

@mtias mtias added [Feature] Block API API that allows to express the block paradigm. [Type] Task Issues or PRs that have been broken down into an individual action to take labels Jun 22, 2017
@youknowriad youknowriad force-pushed the try/generic-block-classnamewq branch from 6c73d48 to 7240717 Compare June 23, 2017 14:30
@youknowriad youknowriad merged commit 6149ef1 into master Jun 23, 2017
@youknowriad youknowriad deleted the try/generic-block-classnamewq branch June 23, 2017 14:44
@nylen
Copy link
Member

nylen commented Jun 29, 2017

@youknowriad why do we drop the core/ namespace here? I think it would be good to have a direct correspondence between the class name and the full block name including namespace.

Alternative suggestion: wp-block__core__text

From docs/design.md:

Key Values
All blocks are created equal, ...

@youknowriad
Copy link
Contributor Author

We had a discussion in Slack (should have reported this here), and we decided to drop the namespace for core blocks to shorten the classname (and we'll forbid the core namespace for third party blocks). cc @mtias

@nylen
Copy link
Member

nylen commented Jun 29, 2017

I think we should revisit this decision. It's a definitive way in which core blocks are not equal to plugin blocks; it imposes additional cognitive load; and anything related to how we handle block names should be super simple, consistent, and explicit.

This breaks down a bit if, for example, a plugin registers a block named text/poetry. Then you end up with classes wp-block-text for core/text and wp-block-text-poetry for text/poetry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement a reliable, consistent, easy to use system for HTML classes
3 participants