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

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

Conversation

@jorgefilipecosta
Copy link
Member

commented Jul 8, 2019

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

Jul-08-2019 17-25-59

@jorgefilipecosta jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch from 7608257 to ad15886 Jul 8, 2019

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

This comment has been minimized.

Copy link
@swissspidy

swissspidy Jul 9, 2019

Member

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?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 9, 2019

Author Member

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

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

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

This comment has been minimized.

Copy link

commented Jul 9, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

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

left a comment

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?
@@ -218,6 +218,29 @@ export function toggleSelection( isSelectionEnabled = true ) {
};
}

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

This comment has been minimized.

Copy link
@mcsf

mcsf Jul 12, 2019

Contributor

autoApplyBlockStyles should be assigned one level up, in getBlocksWithAutoApplyStyles.

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

This comment has been minimized.

Copy link
@mcsf

mcsf Jul 12, 2019

Contributor

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

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

function getBlocksWithAutoApplyStyles( blocks, blockEditorSettings ) {

This comment has been minimized.

Copy link
@mcsf

mcsf Jul 12, 2019

Contributor

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.

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

This comment has been minimized.

Copy link
@mcsf

mcsf Jul 12, 2019

Contributor

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.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 15, 2019

Author Member

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

This comment has been minimized.

Copy link

commented Jul 14, 2019

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.

Select-default-style


Sidebar:
Select-default-style-sidebar

@mtias

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2019

@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 jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch 3 times, most recently from 3f84cf5 to d99116d Jul 15, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

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.

@nosolosw

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

I think that is assuming too much.

@jorgefilipecosta jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch 2 times, most recently from 220e6d3 to 115d47d Jul 26, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2019

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

@jorgefilipecosta jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch from 115d47d to 81bd7fa Aug 6, 2019

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

This comment has been minimized.

Copy link
@mcsf

mcsf Aug 6, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@mcsf

mcsf Aug 6, 2019

Contributor

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.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Aug 6, 2019

Author Member

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.

This comment has been minimized.

Copy link
@mcsf

mcsf Aug 8, 2019

Contributor

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. :)

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 8, 2019

Contributor

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.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 8, 2019

Contributor

sorry for the long comment with no real solution :)

This comment has been minimized.

Copy link
@mcsf

mcsf Aug 8, 2019

Contributor

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?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 8, 2019

Contributor

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?

This comment has been minimized.

Copy link
@mcsf

mcsf Aug 8, 2019

Contributor

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?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Aug 8, 2019

Author Member

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 jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch from 81bd7fa to 199ae64 Aug 6, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

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

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

@mcsf

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@jorgefilipecosta, this needs a rebase.

Updated docs (+3 squashed commits)
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 jorgefilipecosta force-pushed the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch from 698ddcc to 9b0d9b7 Aug 23, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

Hi @mcsf, I rebased this branch.

@mcsf
mcsf approved these changes Aug 26, 2019
Copy link
Contributor

left a comment

Looks good! 🚀

@jorgefilipecosta jorgefilipecosta merged commit 4698c30 into master Aug 26, 2019

2 checks passed

pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details

@jorgefilipecosta jorgefilipecosta deleted the add/mechanism-allow-user-to-configure-an-auto-applied-block-style branch Aug 26, 2019

donmhico added a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
Add: Option to select the style that is automatically applied (WordPr…
…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.
dratwas added a commit to callstack/gutenberg that referenced this pull request Aug 28, 2019
Add: Option to select the style that is automatically applied (WordPr…
…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.
gziolo added a commit that referenced this pull request Aug 29, 2019
Add: Option to select the style that is automatically applied (#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.
gziolo added a commit that referenced this pull request Aug 29, 2019
Add: Option to select the style that is automatically applied (#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.