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: Allow the Default Style selector to be hidden. #20620

Merged
merged 7 commits into from
Mar 19, 2020

Conversation

pento
Copy link
Member

@pento pento commented Mar 4, 2020

Description

While working on Automattic/jetpack#14852, I came across the Default Style dropdown. I don't think it's applicable to how this block uses styles, it would significantly reduce end user cognitive load if we had an option to hide it.

This PR adds such an option, giving blocks a new supports.defaultStyle setting to register with. Setting it to false will remove the default style dropdown.

How has this been tested?

Tested against the block in Automattic/jetpack#14852, and against core blocks which don't have this flag set.

Screenshots

A block with custom styles, and the default style dropdown removed:

A block with custom styles, and the default style dropdown kept:

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@pento pento added [Feature] Block API API that allows to express the block paradigm. Needs Design Feedback Needs general design feedback. [Feature] Theme Style Variations Related to style variations provided by block themes labels Mar 4, 2020
@pento pento self-assigned this Mar 4, 2020
@github-actions
Copy link

github-actions bot commented Mar 4, 2020

Size Change: +31 B (0%)

Total Size: 857 kB

Filename Size Change
build/block-editor/index.js 100 kB +31 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 7.23 kB 0 B
build/block-library/editor.css 7.24 kB 0 B
build/block-library/index.js 111 kB 0 B
build/block-library/style-rtl.css 7.42 kB 0 B
build/block-library/style.css 7.43 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.2 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.2 kB 0 B
build/edit-post/style-rtl.css 8.52 kB 0 B
build/edit-post/style.css 8.51 kB 0 B
build/edit-site/index.js 5.07 kB 0 B
build/edit-site/style-rtl.css 2.53 kB 0 B
build/edit-site/style.css 2.53 kB 0 B
build/edit-widgets/index.js 4.43 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/index.js 44 kB 0 B
build/editor/style-rtl.css 3.97 kB 0 B
build/editor/style.css 3.96 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.09 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.69 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.58 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

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

Nice, this is a welcome change, in an example I was working with the extra selector confused things, this makes it cleaner for sure. 👍

A minor edit for the docs to include the default value is true

@scruffian
Copy link
Contributor

This looks good, but I'm not sure that having it as a new root level setting makes the most sense. There's a few other options I considered:

  1. Change the definition of style settings to contain an object, something like this:
	options: [ ...put existing styles in here ],
	allowDefault: true | false
}```

2. Add it as another property of the `supports` attribute which currently contains similar things, like alignment settings etc...

@youknowriad
Copy link
Contributor

Since this is a new block API let's give it some proper thoughts @WordPress/gutenberg-core

@ZebulanStanphill
Copy link
Member

Personally, I think the default style select control should be moved to the Global Styles UI.

@pento
Copy link
Member Author

pento commented Mar 4, 2020

Giving this some more thought, I agree with @scruffian's point that this isn't really appropriate for a top level setting.

I like the idea of putting it in supports.

@pento pento force-pushed the add/default-style-flag branch from 799ce62 to 0345223 Compare March 4, 2020 23:39
Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

Well this definitely looks a lot simpler now!

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I hadn't followed closely to the original implementation in #16465. It doesn't seem clear to me what guidelines a block developer might want to follow when considering whether to allow this option to be presented. Even in the case of the given "Map" block styles, I think it might be reasonable that a user would want to have a consistent default for the blocks they insert.

That said, I also don't feel particularly strongly that this not be within the control of the block to opt out. In that light, the implementation here seems sensible to me.

@@ -716,6 +716,14 @@ attributes: {
alignWide: false,
```

- `defaultStyle` (default `true`): When the style picker is shown, a dropdown is displayed so the user can select a default style for this block type. If you prefer not to show the dropdown, set this property to `false`.
Copy link
Member

Choose a reason for hiding this comment

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

Noting that the editor setting itself is still marked as experimental:

settings.__experimentalPreferredStyleVariations;

Which leaves this in an awkward mix of semi-stable support.

Related: #16465

@jorgefilipecosta Do you have a sense whether this is something that will plan to be stabilized? Or at least that even if the setting shape itself changes, the desire for "default style" user control will remain?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jorgefilipecosta: Pinging you on this question: is there any plan to stabilise this feature?

Copy link
Member

Choose a reason for hiding this comment

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

We still have a considerable amount of work to do on #7551. Allowing the user to change the default styles was just a step.
We also want to allow themes to change default styles #16532.
And we want to allow users to create their block styles from the UI. I think this API should not be stabilized before at least these two follow-ups are merged and tested (as the follow-up work may benefit from a better mechanism to store and pass default styles around the several artifacts we have). So there are no immediate plans for stabilization.

packages/blocks/CHANGELOG.md Outdated Show resolved Hide resolved
@pablinos
Copy link
Member

pablinos commented Mar 6, 2020

I know this would involve a change to the DefaultSylePicker too, but would it be beneficial to be able to set the default style at registration? Make defaultStyle be either false to disable it, true to have no default set, and a string to set an initial default?

@aduth
Copy link
Member

aduth commented Mar 6, 2020

I know this would involve a change to the DefaultSylePicker too, but would it be beneficial to be able to set the default style at registration? Make defaultStyle be either false to disable it, true to have no default set, and a string to set an initial default?

If I understand correctly, this is already possible via the isDefault property of the registered block style.

https://developer.wordpress.org/block-editor/developers/filters/block-filters/#block-style-variations

At registration-time, it is achieved via the block setting styles property.

Related: #20620 (comment)

@pento
Copy link
Member Author

pento commented Mar 10, 2020

I agree with @aduth's assessment on using defaultStyle to set the default style on registration: this duplicates the isDefault flag.

@aduth: Do you have any further thoughts on this?

@pablinos
Copy link
Member

At registration-time, it is achieved via the block setting styles property.
Related: #20620 (comment)

Of course, I should have read that more closely. Sorry all!

I notice there aren't any safeguards against registering multiple defaults, but that's for another day.

@pento
Copy link
Member Author

pento commented Mar 12, 2020

I'm not entirely sure what the deal is with the __experimentalPreferredStyleVariations flag: it's labelled as experimental, but it's available in stable WordPress releases, so there's a certain point where the "experimental" label becomes obsolete through general use in production.

It would probably be beneficial to formalise that, before it becomes locked in due to general use.

@pento pento force-pushed the add/default-style-flag branch from e769c12 to d48cdbc Compare March 17, 2020 03:22
@pento
Copy link
Member Author

pento commented Mar 17, 2020

@mtias: Can I get your thoughts on this? It's stalled a bit, particularly on what the future of block styles looks like.

@pento pento removed the Needs Design Feedback Needs general design feedback. label Mar 19, 2020
@pento pento added this to the Gutenberg 7.8 milestone Mar 19, 2020
@pento pento merged commit df237e0 into master Mar 19, 2020
@pento pento deleted the add/default-style-flag branch March 19, 2020 04:20
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. [Feature] Theme Style Variations Related to style variations provided by block themes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants