-
Notifications
You must be signed in to change notification settings - Fork 0
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to give this a more specific name like BlockUnsavedChangesIndicators
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 |
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: