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

Docs: clarify isDefault usage for registerBlockStyles() #11478

Merged
merged 4 commits into from Jan 29, 2019

Conversation

@ajitbohra
Copy link
Member

ajitbohra commented Nov 5, 2018

Removes isDefault info for registerBlockStyle() from handbook.

Clarify isDefault usage for registerBlockStyles()

Reference #11451

@tofumatt

This comment has been minimized.

Copy link
Member

tofumatt commented Nov 5, 2018

Hi @ajitbohra, thanks for the PR. In the future, feel free to assign more folks than just @chrisvanpatten and I to review documentation PRs. It's probably best to assign the review to gutenberg-core, so that a wider group of people can see the review request 😄

@tofumatt
Copy link
Member

tofumatt left a comment

I can't tell from the issue if there is a bug here or not; in the issue it seems like @dgwyer is saying this should work. So I don't think removing the docs is the right approach.

@dgwyer

This comment has been minimized.

Copy link

dgwyer commented Nov 5, 2018

I think it makes sense to have the isDefault property in registerBlockStyle() override the default style variation that may have been set in the blocks registerBlockType() function.

The issue is that isDefault appears to be ignored when used in registerBlockStyle().

@tofumatt

This comment has been minimized.

Copy link
Member

tofumatt commented Nov 5, 2018

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 5, 2018

I think people are confusing "isDefault" as the default style variation applied when you insert the block (concept we don't have at the moment). That's not what the current "isDefault" refer to. The current "isDefault" is used to give a name to the style variation that doesn't add any custom className. (The no-style style variation).

@ajitbohra

This comment has been minimized.

Copy link
Member Author

ajitbohra commented Nov 6, 2018

@tofumatt noted agree with you on that

@youknowriad yes currently it's confusing if I am not wrong its kind of fallback style in case of missing style variation?

Instead of removing we need copy update for the isDefault

@dgwyer

This comment has been minimized.

Copy link

dgwyer commented Nov 20, 2018

@youknowriad I think I'm more confused now than when I started. Here's what I found after more testing.

It appears that when isDefault is used inside registerBlockStyle() it's recognized ONLY if no styles in registerBlockType() have been set as the default. Otherwise isDefault is always ignored inside of registerBlockStyle().

Also, even if a style variation is selected in the block UI on insertion no CSS class is actually applied to the block wrapper element until a style variation option is manually clicked.

This feels counter intuitive to me. I'd expect the style variation CSS class to be applied whenever one is selected, either on block insertion or manual selection. This isn't currently the case.

And allowing registerBlockStyle() to override the default style variation defined in registerBlockStyle() would make a lot of sense.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 20, 2018

@dgwyer yes, at the moment isDefault is just used to give a name the "no class" style variation. It's not meant to solve the style variation selected when you inserter the block use-case, this "concept" doesn't exist yet.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 25, 2019

@ajitbohra can you clarify the intent of isDefault instead of removing this line based on this comment from @youknowriad:

The current "isDefault" is used to give a name to the style variation that doesn't add any custom className. (The no-style style variation).

@ajitbohra ajitbohra self-assigned this Jan 27, 2019

@ajitbohra ajitbohra changed the title Docs: Remove isDefault from registerBlockStyle Docs: clarify isDefault usage for registerBlockStyles() Jan 27, 2019

@ajitbohra

This comment has been minimized.

Copy link
Member Author

ajitbohra commented Jan 27, 2019

@gziolo PR updated to clarify the usage of isDefault

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 28, 2019

@youknowriad - is it correct now?

@@ -17,7 +17,7 @@ wp.blocks.registerBlockStyle( 'core/quote', {

The example above registers a block style variation named `fancy-quote` to the `core/quote` block. When the user selects this block style variation from the styles selector, an `is-style-fancy-quote` className will be added to the block's wrapper.

By adding `isDefault: true`, you can make registered style variation to be active by default when a block is inserted.
By adding `isDefault: true` you can make the registered style variation to be used as a fallback if no style in `registerBlockType()` has been set as the default.

This comment has been minimized.

@youknowriad

youknowriad Jan 28, 2019

Contributor

Maybe something like?

By adding isDefault: true you can mark the registered style variation as the one that is active without any custom className.

(Or something in that vein, it's hard to explain :P)

Clarify isDefault
Co-Authored-By: ajitbohra <ajit@lubus.in>
@gziolo

gziolo approved these changes Jan 29, 2019

Copy link
Member

gziolo left a comment

Haha, so @ajitbohra couldn't approve. Let me do it on his behalf as I authored the last version. Thanks for good teamwork on this one 💯

@gziolo gziolo merged commit a00c7f4 into master Jan 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the update/docs-block-styles branch Jan 29, 2019

@youknowriad youknowriad added this to the Documentation & Handbook milestone Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment