Add: Option to select the style that is automatically applied #16465

jorgefilipecosta · 2019-07-08T16:46:10Z

Part of #7551.
This PR's adds a mechanism that allows users to select a style that is automatically added when a given block is added.

Description

How has this been tested?

I went to the quote block, I opened the block switcher, I verified each style contains an auto apply option.
I selected the "Auto apply" option for the large style.
I inserted a new quote block I verified the Large style was applied.
I inserted a paragraph and I added some text. I transformed the paragraph to a quote and I verified the large style was used.
I reloaded the editor and I verified the auto apply setting was persisted.
I did general smoke testing to the block insert and transform actions.

Screenshots

swissspidy · 2019-07-09T08:53:40Z

packages/block-editor/src/components/block-styles/index.js

+						{ onUpdateAutoApplyBlockStyles && (
+							<CheckboxControl
+								className="block-editor-block-styles__item-auto-apply"
+								label={ __( 'Auto apply' ) }


I feel like this needs better wording. Maybe something more simple like "Use as default".

Also, how well does this work with i18n? This string's translation might easily take up two lines. Does it still look good in that case?

Maybe something more simple like "Use as default".

I thought about that, but the problem is that we call our main style Default, so using "Use as default" would be confusing. We can and should change the current default styles to a name like "Standard" but other blocks may still use "Default" in their names.

Also, how well does this work with i18n? This string's translation might easily take up two lines. Does it still look good in that case?

Well remembered I will double check with bigger strings.

youknowriad · 2019-07-09T10:25:49Z

One of the ideas that we may explore is the possibility for themes (or plugins) to define the default style that is applied. A question to be asked here is how these two "default" settings work together.
Should the user one take precedence in that case?

cc @richtabor @mtias

richtabor · 2019-07-09T16:01:39Z

The idea is interesting but I'm unsure of what the proper experience should be around the control.

This feels out of place within the Styles panel, as we're introducing a whole new interaction to the inserter, which does not exist elsewhere in any panels of the same sort.

A question to be asked here is how these two "default" settings work together.
Should the user one take precedence in that case?

User's selection should have the priority. If a theme/plugin gains the ability to select a default (which I think is a marvelous idea!) and then a user selects a different default, the user's selection should take precedence for that particular block's default style from that point on.

jorgefilipecosta · 2019-07-09T18:28:11Z

Thank you for your inputs @richtabor, @youknowriad, and @swissspidy.

The idea is interesting but I'm unsure of what the proper experience should be around the control.

This feels out of place within the Styles panel, as we're introducing a whole new interaction to the inserter, which does not exist elsewhere in any panels of the same sort.

I had the same concern, other option I thought about would be to use a modal just for default style selection, similar to the block manager, part of the editor settings. I did not follow this option because I had concerns users would not discover this option exist, while on the actual position it is very easy to find. Maybe the best option would be a mix of the two? I'm totally open to trying different UI's.

User's selection should have the priority. If a theme/plugin gains the ability to select a default (which I think is a marvelous idea!) and then a user selects a different default, the user's selection should take precedence for that particular block's default style from that point on.

I agree with this logic. I think we can merge this PR first which adds the mechanism to use a style by default, and then we can add a theme defaults options that follow this logic.

mcsf

This looks good in terms of architecture. Caveats:

I'm not very convinced by the UX, but I don't feel qualified to suggest anything better.
Let's make sure that we have the i18n scenarios figured out, as suggested by swissspidy.
A future-proof solution should also answer the question of how we could implement "retroactive" auto-styles, i.e. how can I take a long post, change the style of one of its quotes, and ask the editor to make all the quotes in the current post of the same style?

mcsf · 2019-07-12T14:01:06Z

packages/block-editor/src/store/actions.js

@@ -218,6 +218,29 @@ export function toggleSelection( isSelectionEnabled = true ) {
 	};
 }

+function getBlocksWithAutoApplyStyles( blocks, blockEditorSettings ) {
+	return blocks.map( ( block ) => {
+		const autoApplyBlockStyles = get( blockEditorSettings, [ 'autoApplyBlockStyles' ], {} );


autoApplyBlockStyles should be assigned one level up, in getBlocksWithAutoApplyStyles.

mcsf · 2019-07-12T14:01:37Z

packages/block-editor/src/store/actions.js

+		if ( includes( className, 'is-style-' ) ) {
+			return block;
+		}
+		const attributes = block.attributes || {};


Minor: prefer const { attributes = {} } = block;

packages/block-editor/src/store/test/actions.js

mcsf · 2019-07-12T14:14:43Z

packages/block-editor/src/store/actions.js

@@ -218,6 +218,29 @@ export function toggleSelection( isSelectionEnabled = true ) {
 	};
 }

+function getBlocksWithAutoApplyStyles( blocks, blockEditorSettings ) {


Since we neatly separate the getting of blockEditorSettings from the logic of getBlocksWithAutoApplyStyles, we should add a couple of unit tests for this, as well as add some JSDoc for the function.

mcsf · 2019-07-12T14:24:25Z

packages/block-editor/src/store/actions.js

+			'core/block-editor',
+			'getSettings',
+		)
+	);


One question on my mind is whether we should log to the console any instances of auto-applying, so that anyone debugging their blocks can be aware of this transformation of blocks data.

Note that this is no substitute to a UI that is very clear in what is happening.

We have some history in printing debugging information on the console for raw handling debugging. I'm not sure if for this case in concrete we should print a message, developers will soon know default styles can be changed by theme and the user so it will be something expected.

paaljoachim · 2019-07-14T11:29:28Z

It looks interesting as the user can easily select a "default" style used for any given block that has styles included. One needs also to include the "default" style checkbox in the sidebar under the styles section. One thing I am hesitant about is having a "default" checkbox under each style.
Something like the below might work.

Sidebar:

mtias · 2019-07-14T13:32:02Z

@jorgefilipecosta let's make this separate from normal style selection. This is already a bit of a secondary setting, so let's move it to the block inspector only to start.

Let's also decouple it a bit from the style grid. We can start with a select control. We should also explain a bit better in text what happens when you change it:

jorgefilipecosta · 2019-07-15T15:16:01Z

Hi @paaljoachim, @mcsf, and @mtias thank you for the feedback/insights.
@mtias the design was updated, and your suggestion was implemented.
@mcsf I applied your ideas, only unit tests are pending, and I plan to add them very soon.
Regarding:

A future-proof solution should also answer the question of how we could implement "retroactive" auto-styles, i.e., how can I take a long post, change the style of one of its quotes, and ask the editor to make all the quotes in the current post of the same style?

I guess we can have a button on the styles panel called "Apply to all blocks" or something similar, that applies the currently selected style to every block of the same type? I think this is not directly related to the default styles (e.g.: it may make sense to apply a non-default style to every block in a given post), so maybe we can leave this feature to other PR.

oandregal · 2019-07-18T10:56:58Z

From an UX perspective: how about we don't have a separate control for setting the default style but we use the last style chosen by the user as the default?

The rationale for this would be that the style chosen for a specific block is highly likely to be used for the next block of the same type as well. Eg: if you add a list block with a square style, the next list block you add you'll probably want it to be square as well - and not circle or whatever is the default set by core or the themes/plugins.

mcsf · 2019-07-18T15:22:06Z

From an UX perspective: how about we don't have a separate control for setting the default style but we use the last style chosen by the user as the default?

I think there's merit to that idea. Less UI, more sensible inferring of intent.

mtias · 2019-07-18T15:23:21Z

I think that is assuming too much.

jorgefilipecosta · 2019-07-27T12:16:25Z

Test cases were the UI is updated I think this PR should be ready.

mcsf · 2019-08-06T14:39:12Z

packages/block-editor/src/components/default-style-picker/index.js

+			[ { label: __( 'Not set' ), value: '' } ]
+		).concat(
+			styles.map( ( { label, name } ) => ( { label, value: name } ) )
+		),


Array spread notation could make this more readable than concat: [ { … }, ...styles.map( … ) ]

mcsf · 2019-08-06T15:04:22Z

packages/block-editor/src/components/default-style-picker/index.js

+			const settings = select( 'core/block-editor' ).getSettings();
+			return {
+				defaultStyle: get( settings, [ 'defaultBlockStyles', blockName ] ),
+				onUpdateDefaultBlockStyles: settings.onUpdateDefaultBlockStyles,


This to me is quite strange. Have we discussed the APIs to set user preferences and exposing them in Settings like this? Otherwise I'd expect this change to be applied in core components by calling our dispatcher API.

Hi @mcsf,

Have we discussed the APIs to set user preferences and exposing them in Settings like this? Otherwise I'd expect this change to be applied in core components by calling our dispatcher API.

I think this was not discussed before. The edit-post module is handling the block style preference, the UI to change the style is part of the style picker (block editor module). The block editor module should not be aware of the edit post module, so it can not use our dispatcher API to update the preference. Keeping this abstraction allows users of the block editor module that may not use our data module to take advantage of the default style changer UI easily.
We have not yet a case similar to this one, but passing functions as settings to bock editor is not something new. For example, we pass the function that allows media uploads as a setting. The cases are not that different as both use a function that allows the block editor to change some state external to it (add a media file to the WordPress media lib for media uploads, change default block style and save on local storage for default style change).

Another option would be passing onUpdateDefaultBlockStyles as an optional property of the BlockEditor component, but that increases the complexity even more as we would need to use context to pass this property does down to the UI components that apply the change.

Thanks for the reply, @jorgefilipecosta.

The edit-post module is handling the block style preference, the UI to change the style is part of the style picker (block editor module). The block editor module should not be aware of the edit post module, so it can not use our dispatcher API to update the preference. Keeping this abstraction allows users of the block editor module that may not use our data module to take advantage of the default style changer UI easily.

So in my mind we'd use some of our Data features such as controls and registry awareness so that DefaultStylePicker only interacts with block-editor data, in particular by dispatching through block-editor, while behind the scenes the block-editor store would bridge with editor. The key here would be to not make assumptions about unicity of those stores, and I'm not familiar enough with the current state of our data layer to say how feasible this is. @youknowriad, do you know?

We have not yet a case similar to this one, but passing functions as settings to bock editor is not something new. For example, we pass the function that allows media uploads as a setting.

I had a quick glance at the other objects exposed as settings and failed to spot those functions — my bad.

Another option would be passing onUpdateDefaultBlockStyles as an optional property of the BlockEditor component, but that increases the complexity even more as we would need to use context to pass this property does down to the UI components that apply the change.

Yeah, probably not worth it.

Taking a step back, I think Settings are a bit of chimera, in that they are "just" store data that's passed around but it's made available through separate channels, usually premised on remembering users' preferences. That is, it's the preferences themselves which are the settings, not the functions by which preferences are set. That's why it didn't seem right to me to expose the functions. That said, if we have precedents for this then I don't want to keep this PR from moving. :)

I don't think we have a good answer here tbh. Some thoughts though:

So in my mind we'd use some of our Data features such as controls and registry awareness so that DefaultStylePicker only interacts with block-editor data, in particular by dispatching through block-editor, while behind the scenes the block-editor store would bridge with editor. The key here would be to not make assumptions about unicity of those stores, and I'm not familiar enough with the current state of our data layer to say how feasible this is. @youknowriad, do you know?

There's no straight-forward API to do that at the moment. It might be possible to achieve by duplicating the data in two stores (subcribing and redispatching on change) but it feels wrong. That said, I'm not certain that's the perfect API for that use case.

Thinking of the BlockEditor usage outside of the WordPress context help us think about the best API here I think.

So far, we have four main ways to "extend/use" the block editor: settings, composing UI components, slots and filters. In my opinion, from a third-party developer's perspective, the best devX would be:

Compose my own UI

Provide settings

Slots comes after as while it's useful sometimes, if I'm able to compose my own UI, I shouldn't need them as much.

Filters for very specific numbers/strings... we want to be tweakable

In this particular use-case, my first question is how valuable this feature/setting would be outside of WordPress? If it's something users would want to tweak often, I think the "settings" might be the best place for it. mediaUpload is a good example of something that any third party editor might want to tweak while keeping the same blocks...

It doesn't feel like something too common for me so I'd personally try to find the less impacting the block-editor module. It doesn't mean I won't use settings for it though especially if I can find a good "setting property" for it. It looks like it's composed of two settings: the value of the settings and a change handler. Also how can we disable it/enable it, is it by the presence of the handler. What if it's something like:

preferedStyleVariations: { value, onChange }

and the presence of the whole object indicate support or not?

I'm still not convinced totally yet though :) I think the main problem we're facing here is that the BlockEditorProvider component is thought to be a component that edits one value: The blocks. So it has a value prop for the blocks and an onChange handler. And the settings are just a way to tweak its behavior. In this Particular PR we're making the block editor edit a secondary "value", the preferred styles.

sorry for the long comment with no real solution :)

Thanks, I think that helps put things in perspective.

Since there's no solid path forward, are we OK with proceeding with the current solution?

I'm personally fine with it. We should just have this UI disabled by default if you use the block editor without these settings. We could consider marking it experimental?

Aside: Do you think it's possible to build this as a Slot/UI component in the editor module instead? Conceptually speaking, this would mean we're just editing a data of the post editor but not the block editor, we're just trying to "merge" the UI. Did we give any thoughts to that possibility?

I'm personally fine with it. We should just have this UI disabled by default if you use the block editor without these settings. We could consider marking it experimental?

Yes, let's make it experimental.

Aside: Do you think it's possible to build this as a Slot/UI component in the editor module instead? Conceptually speaking, this would mean we're just editing a data of the post editor but not the block editor, we're just trying to "merge" the UI. Did we give any thoughts to that possibility?

That sounds pretty interesting, I'd like to see it tried.

@jorgefilipecosta, what do you think of first moving forward by marking the exposed bits from this PR as experimental? Later, do you think it's worth exploring the editor-side Slot/Fill approach?

Hi @youknowriad, @mcsf,
I updated the code marked it as experimental and followed @youknowriad suggestion:

preferedStyleVariations: { value, onChange }

I thought about the Slot Fill approach, basically, add a slot to the styles panel, but that slot seemed arbitrary. Why it only appears in the styles panel inside block inspector and not in styles inside block switcher? Why we have a slot in this panel but not other panels?
We may have cases of multiple block editors, how do I enable style change functionality in some of the block editors but not others?
This approach allows us to pass different change handlers per block editor the slot fill seemed more limited and more complex.

jorgefilipecosta · 2019-08-06T15:40:41Z

Hi @mcsf, thank you for the review. Your suggestion was applied and the comment/question answered.

mcsf · 2019-08-22T15:19:08Z

@jorgefilipecosta, this needs a rebase.

Squashed commits: [ffd16b5] Add unit tests [51dd9dc] Refactor & changed the design [3b9ac24] Add: Option to allow user to select the style that is automatically applied.

jorgefilipecosta · 2019-08-23T16:00:00Z

Hi @mcsf, I rebased this branch.

mcsf

Looks good! 🚀

…ess#16465) Squashed commits: [ffd16b5] Add unit tests [51dd9dc] Refactor & changed the design [3b9ac24] Add: Option to allow user to select the style that is automatically applied.

Squashed commits: [ffd16b5] Add unit tests [51dd9dc] Refactor & changed the design [3b9ac24] Add: Option to allow user to select the style that is automatically applied.

jorgefilipecosta requested review from ellatrix, gziolo, talldan and youknowriad as code owners July 8, 2019 16:46

jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch from 7608257 to ad15886 Compare July 8, 2019 21:00

swissspidy reviewed Jul 9, 2019

View reviewed changes

jorgefilipecosta added [Feature] Theme Style Variations Related to style variations provided by block themes Needs Design Feedback Needs general design feedback. labels Jul 9, 2019

jorgefilipecosta mentioned this pull request Jul 11, 2019

Add option for themes to configure block styles #16532

Closed

mcsf reviewed Jul 12, 2019

View reviewed changes

jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch 3 times, most recently from 3f84cf5 to d99116d Compare July 15, 2019 15:06

jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch 2 times, most recently from 220e6d3 to 115d47d Compare July 27, 2019 09:52

jorgefilipecosta requested review from ajitbohra, chrisvanpatten and mkaz as code owners July 27, 2019 09:52

jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch from 115d47d to 81bd7fa Compare August 6, 2019 13:25

mcsf reviewed Aug 6, 2019

View reviewed changes

jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch from 81bd7fa to 199ae64 Compare August 6, 2019 15:37

jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch 2 times, most recently from c2ad91f to 698ddcc Compare August 8, 2019 15:29

Updated docs (+3 squashed commits)

9b0d9b7

Squashed commits: [ffd16b5] Add unit tests [51dd9dc] Refactor & changed the design [3b9ac24] Add: Option to allow user to select the style that is automatically applied.

jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch from 698ddcc to 9b0d9b7 Compare August 23, 2019 15:58

mcsf approved these changes Aug 26, 2019

View reviewed changes

jorgefilipecosta merged commit 4698c30 into master Aug 26, 2019

jorgefilipecosta deleted the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch August 26, 2019 10:56

aduth mentioned this pull request Mar 5, 2020

Blocks: Allow the Default Style selector to be hidden. #20620

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add: Option to select the style that is automatically applied #16465

Add: Option to select the style that is automatically applied #16465

jorgefilipecosta commented Jul 8, 2019

swissspidy Jul 9, 2019

jorgefilipecosta Jul 9, 2019

youknowriad commented Jul 9, 2019

richtabor commented Jul 9, 2019

jorgefilipecosta commented Jul 9, 2019 •

edited

Loading

mcsf left a comment

mcsf Jul 12, 2019

mcsf Jul 12, 2019

mcsf Jul 12, 2019

mcsf Jul 12, 2019

jorgefilipecosta Jul 15, 2019

paaljoachim commented Jul 14, 2019 •

edited

Loading

mtias commented Jul 14, 2019 •

edited

Loading

jorgefilipecosta commented Jul 15, 2019

oandregal commented Jul 18, 2019

mcsf commented Jul 18, 2019

mtias commented Jul 18, 2019

jorgefilipecosta commented Jul 27, 2019

mcsf Aug 6, 2019

mcsf Aug 6, 2019

jorgefilipecosta Aug 6, 2019

mcsf Aug 8, 2019

youknowriad Aug 8, 2019

youknowriad Aug 8, 2019

mcsf Aug 8, 2019

youknowriad Aug 8, 2019

mcsf Aug 8, 2019

jorgefilipecosta Aug 8, 2019

jorgefilipecosta commented Aug 6, 2019

mcsf commented Aug 22, 2019

jorgefilipecosta commented Aug 23, 2019

mcsf left a comment

Add: Option to select the style that is automatically applied #16465

Add: Option to select the style that is automatically applied #16465

Conversation

jorgefilipecosta commented Jul 8, 2019

Description

How has this been tested?

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jul 9, 2019

richtabor commented Jul 9, 2019

jorgefilipecosta commented Jul 9, 2019 • edited Loading

mcsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paaljoachim commented Jul 14, 2019 • edited Loading

mtias commented Jul 14, 2019 • edited Loading

jorgefilipecosta commented Jul 15, 2019

oandregal commented Jul 18, 2019

mcsf commented Jul 18, 2019

mtias commented Jul 18, 2019

jorgefilipecosta commented Jul 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Aug 6, 2019

mcsf commented Aug 22, 2019

jorgefilipecosta commented Aug 23, 2019

mcsf left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Jul 9, 2019 •

edited

Loading

paaljoachim commented Jul 14, 2019 •

edited

Loading

mtias commented Jul 14, 2019 •

edited

Loading