Add a dot before the parent block selector when there are unsaved changes in the Reusable block #1
Conversation
|
Size Change: +601 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
The issue I mentioned below with using core data means that we'll need to think of a different more generic way to achieve this.
Something like a slot/fill could be an option, the reusable block edit function could render a component from the block-editor package that uses a slot/fill internally to show up in the BlockParentSelector button.
It might be worth also running the idea past some other developers to see what ideas they have.
packages/block-editor/package.json
Outdated
| @@ -38,6 +38,7 @@ | |||
| "@wordpress/blocks": "file:../blocks", | |||
| "@wordpress/components": "file:../components", | |||
| "@wordpress/compose": "file:../compose", | |||
| "@wordpress/core-data": "file:../core-data", | |||
There was a problem hiding this comment.
I think this dependency might be a bit of an issue, as core-data is a WordPress utility library, and the block editor package is supposed to be for general use even outside of WordPress.
There was a problem hiding this comment.
I was not aware of this 😅
|
@talldan Thank you for taking a look 😄 I will try to implement it with the slot/fill option. Maybe this can be added as an improvement after WordPress#32464 gets merged? |
There was a problem hiding this comment.
This is going in the right direction, thanks @thisissandip, it's not an easy thing to get right.
I think the new components in this PR should be made experimental (exported from the package's index.js with the __experimental prefix). The reason for that is it indicates the component is not yet stable. The idea of these dots for unsaved changes indicators is still in a very early stage, so I don't think it's something we'd want to promote to plugin developers just yet while the UI might yet change.
| <Slot | ||
| name="parent-selector-has-dot" | ||
| fillProps={ { | ||
| setReusableBlockHasEdits, | ||
| } } | ||
| ></Slot> |
There was a problem hiding this comment.
I don't think there's a need to use the slot to set a classname on the parent div. React components can be rendered in a slot, a bit like a portal.
Ideally this is named something more semantic, like 'parent-block-unsaved-changes-indicator'.
BlockParentSelector should also not have any code that mentions the concept of a reusable block, it should be implemented as a general feature if possible. It may also be useful for the template part block, which is very similar to a reusable block.
I think the Slot should also be rendered as a self closing tag:
<Slot name="parent-selector-has-dot" />There was a problem hiding this comment.
I don't think there's a need to use the slot to set a classname on the parent div. React components can be rendered in a slot, a bit like a portal.
The class is added to increase the BlockParentSelector width and to adjust its left position when the dot indicator is present. Since BlockParentSelector is positioned absolutely inside the block toolbar.
Similar logic is used to increase the left margin of the BlockContextualToolbar to make room for the parent selector button when the dot indicator is present.
Is there some other way to conditionally load styles for BlockContextualToolbar and BlockParentSelector when the BlockUnsavedChangesIndicator is mounted?
There was a problem hiding this comment.
I was able to implement this using useEffect inside the ParentBlockSelectorUnsavedChangesIndicator component
| */ | ||
| import { Fill } from '@wordpress/components'; | ||
|
|
||
| function BlockHasDot( { |
There was a problem hiding this comment.
It'd be good to give this a more specific name like BlockUnsavedChangesIndicators.
There was a problem hiding this comment.
Yes, this makes the most sense.
| <Fill name="parent-selector-has-dot"> | ||
| { ( { setReusableBlockHasEdits } ) => | ||
| parentReusableBlockHasEdits | ||
| ? setReusableBlockHasEdits( true ) | ||
| : setReusableBlockHasEdits( false ) | ||
| } | ||
| </Fill> |
There was a problem hiding this comment.
Wrapping the fill in a component like this is close to what I was thinking, but it should be possible to render the dot directly within the fill. Something like:
<Fill name="parent-selector-has-dot">
<UnsavedChangesIcon />
</Fill>(RichTextToolbarButton is an example of a component that does this.)
| parentReusableBlockHasEdits, | ||
| selectedReusableBlockHasEdits, |
There was a problem hiding this comment.
This is another place where it'd be good to make this more generic, so it isn't just a feature that only reusable blocks can take advantage of.
I don't think these props are needed, since the reusable block code itself can optionally render these. It might require splitting this into two separate components (one for the parent block selector, one for the current block), but something like the following code in the reusable block edit function should achieve it:
( isSelected && hasEdits && {
<SelectedBlockUnsavedChangesIndicator />
} )
( hasChildBlockSelected && hasEdits && {
<ParentBlockSelectorUnsavedChangesIndicator />
} )There was a problem hiding this comment.
The props were used to update the classes of BlockContextualToolbar and the BlockParentSelector.
Is there some other way to conditionally load styles for BlockContextualToolbar and BlockParentSelector when the BlockUnsavedChangesIndicator is mounted?
And yes, splitting them into two separate components is a much better approach. I will update this in the next commit.
Thank you for the review! 🚀
| @@ -147,6 +148,9 @@ export const BlockSwitcherDropdownMenu = ( { clientIds, blocks } ) => { | |||
| } } | |||
| icon={ | |||
| <> | |||
| { isReusable && ( | |||
There was a problem hiding this comment.
I think a slot renders nothing if there are no fills, so it should be fine to just render the slot without the surrounding isReusable logic.
| useEffect( () => { | ||
| // add class to the parent selector to increase it's width | ||
| const parentSelectorBtn = document.querySelector( | ||
| '.block-editor-block-parent-selector' | ||
| ); | ||
| parentSelectorBtn.classList.add( 'parent-block-has-changes' ); | ||
| // add class to the contextual toolbar to move it to the right side | ||
| const contextualToolBar = document.querySelector( | ||
| '.block-editor-block-contextual-toolbar' | ||
| ); | ||
| contextualToolBar.classList.add( 'parent-block-has-changes' ); | ||
|
|
||
| return () => { | ||
| // remove classes from the parent selector and contextual toolbar when component unmounts | ||
| document | ||
| .querySelector( '.block-editor-block-parent-selector' ) | ||
| .classList.remove( 'parent-block-has-changes' ); | ||
| document | ||
| .querySelector( '.block-editor-block-contextual-toolbar' ) | ||
| .classList.remove( 'parent-block-has-changes' ); | ||
| }; | ||
| }, [] ); |
There was a problem hiding this comment.
I'm not in favour of the modification of other parent elements when this renders. Is there a way to achieve the same result without that being required?
There was a problem hiding this comment.
The unsaved changes indicator increases the width of the parent selector button. Since the parent selector button is positioned absolutely to the contextual toolbar, it leads to a state like this:
That's why a new class block-has-changes is added to the contextual toolbar to handle the alignment issues.
I am not sure how to achieve this without affecting the contextual toolbar. If you have any suggestions, I will be more than happy to try them out 🙂
There was a problem hiding this comment.
Since the parent selector button is positioned absolutely to the contextual toolbar, it leads to a state like this
Interesting that this is positioned absolutely. That sounds like the core of the problem. Is there a way to style it so that it's an inline element?
There was a problem hiding this comment.
@thisissandip I think this is in a good state to merge into your other branch, thanks for all the hard work. It'd be good to explore whether the styling issues we're discussing with the parent selector button can be improved in the add/snackbar-and-guide-with-resuable-block-save-button branch.
|
It looks like there was an environment issue with the tests, lets see what happens when this is merged into the |
thisissandip
merged commit 2a213ee
into
add/snackbar-and-guide-with-resuable-block-save-button
Description
While working on WordPress#32464 it was proposed to add a dot before the parent block selector to guide the user about the unsaved changes in the Reusable block.
How has this been tested?
Screenshots
ReusableBlockHasChanges.mov
Types of changes
New feature
Checklist: