Skip to content

Docs: clarify isDefault usage for registerBlockStyles()#11478

Merged
gziolo merged 4 commits intomasterfrom
update/docs-block-styles
Jan 29, 2019
Merged

Docs: clarify isDefault usage for registerBlockStyles()#11478
gziolo merged 4 commits intomasterfrom
update/docs-block-styles

Conversation

@ajitbohra
Copy link
Copy Markdown
Member

@ajitbohra ajitbohra commented Nov 5, 2018 •
edited
Loading

Removes isDefault info for registerBlockStyle() from handbook.

Clarify isDefault usage for registerBlockStyles()

Reference #11451

@ajitbohra ajitbohra added the [Type] Developer Documentation Documentation for developers label Nov 5, 2018
@tofumatt
Copy link
Copy Markdown
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
tofumatt requested changes Nov 5, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Member

tofumatt commented Nov 5, 2018 via email

@youknowriad
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Contributor

dgwyer commented Nov 20, 2018 •
edited
Loading

@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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member Author

ajitbohra commented Jan 27, 2019 •
edited
Loading

@gziolo PR updated to clarify the usage of isDefault

@gziolo
Copy link
Copy Markdown
Member

gziolo commented Jan 28, 2019

@youknowriad - is it correct now?

youknowriad
youknowriad reviewed Jan 28, 2019
View reviewed changes
Comment thread docs/designers-developers/developers/filters/block-filters.md Outdated
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.
Copy link
Copy Markdown
Contributor

@youknowriad youknowriad Jan 28, 2019

Choose a reason for hiding this comment

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

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)

gziolo
gziolo reviewed Jan 28, 2019
View reviewed changes
Comment thread docs/designers-developers/developers/filters/block-filters.md Outdated
@gziolo @ajitbohra
Clarify isDefault
dfb51a0 
Co-Authored-By: ajitbohra <ajit@lubus.in>
gziolo
gziolo approved these changes Jan 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

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
@gziolo gziolo deleted the update/docs-block-styles branch January 29, 2019 09:10
@youknowriad youknowriad added this to the Documentation & Handbook milestone Jan 29, 2019
@gziolo gziolo mentioned this pull request Jan 29, 2019
Closed
This was referenced Feb 4, 2019
Merged
Closed
Merged
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@ajitbohra @youknowriad
Docs: clarify isDefault usage for registerBlockStyles() (#11478)
4b96af1 
* Docs: Remove isDefault from registerBlockStyle

* Docs: clarify isDefault usage for registerBlockStyles()

* Clarify isDefault

Co-Authored-By: ajitbohra <ajit@lubus.in>
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@ajitbohra @youknowriad
Docs: clarify isDefault usage for registerBlockStyles() (#11478)
1f84655 
* Docs: Remove isDefault from registerBlockStyle

* Docs: clarify isDefault usage for registerBlockStyles()

* Clarify isDefault

Co-Authored-By: ajitbohra <ajit@lubus.in>
@bph bph mentioned this pull request Jul 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowriad youknowriad youknowriad left review comments

@tofumatt tofumatt tofumatt requested changes

@gziolo gziolo gziolo approved these changes

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

Assignees

@ajitbohra ajitbohra

Labels

[Type] Developer Documentation Documentation for developers

Projects

None yet

Milestone

Documentation & Handbook

Development

Successfully merging this pull request may close these issues.

5 participants

@ajitbohra @tofumatt @dgwyer @youknowriad @gziolo