-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 Reusable block save button, snackbar on save and Welcome Guide #32464
Add Reusable block save button, snackbar on save and Welcome Guide #32464
Conversation
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.
Thanks for working on this PR, @thisissandip.
I left few comments that I think would be nice to address before merging.
I'm not 100% sure about the Welcome Guide. Maybe the "Learn More" in snackbar should link to external documentation?
Nice! Thanks for working on this @thisissandip. I can create the GIF asset for the Welcome Guide — will follow up on this next week. A couple of notes on the PR:
|
@Mamaduka Thank you for the review. I have made the required changes. Let me know if this looks good! 😄 |
@critterverse Thank you for taking a look, I have updated the code. Those issues are now fixed. ReusableBlock_Save.movLooking forward to that GIF! 😄 |
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.
Thanks for the follow-up, @thisissandip.
I left few minor comments, and I think we're good to land after this.
I'm still not 100% sure about the "Welcome Guide." In my experience, editors display similar guides when the user accesses the screen for the first time.
I general, I think it's a nice addition, just not sure if it's the right place. So I would love to get another opinion before we move forward with this.
cc @jasmussen
Thanks for the PR, thanks for the ping. This is what I see: I love the work @critterverse has done here, and this PR sticks to that very impressively, good work all around. A few thoughts:
It seems the primary open question is whether it's okay to open a reusable blocks guide from the snackbar, compared to opening it automatically when you edit a reusable block the first time. In principle @Mamaduka is right, for the time being we've only done the latter. At the same time, I appreciate @critterverse's solution on a personal level: whenever I see a "welcome guide" like this, I immediately and quickly dismiss, almost as if it was an advertisement. For me, the extra friction of a click is appreciated. I can understand it also might be enough friction that no-one will ever see that guide and simply ignore that "Read more" link. But since this is working well, it seems fine to land this behavior and get a feel for it in the plugin: we can always double back and tweak it if it doesn't work as well in practice. Thanks again for the work here! 👏 |
Thanks for the feedback, @jasmussen.
I've also acquired this habit 😄 , but I assume this is helpful for new users who don't spend every day in the editor. Welcome Modal assets are now hosted on WP.org. I'll create a new ticket on meta once we're close to land this PR. |
Thanks for working on this feature, @thisissandip Various adjustments / new features to the Reusable block needs to also be considered for possible backporting to WP 5.7.3. A question that comes up... Btw I do think there should be a vertical black separator line between the Save button and the ellipsis (3 dot) drop down menu. As the ellipsis drop down should be separate from other toolbar icons. Associated issue: Another issue where I am rethinking various Reusable block features: |
I have moved the Save button to its own group as suggested by @jasmussen and @paaljoachim. Also, made the suggested changes by @Mamaduka. Thank you guys for reviewing it! 😄 SaveBtnReusableBlock.mov |
@paaljoachim Thank you for taking a look 😄 I just went through issue #32461. |
That is awesome! Yes, do please create a new PR for the lock/unlock feature. |
Thanks for the feedback, all! @jasmussen's note about adding the additional divider to the toolbar is a great call and the change you already made there looks perfect, @thisissandip. Just wanted to highlight one other note that I think we should take a look at:
The default focus style looks like this for reference (see also this link to the relevant section of WP Design Library): Here's what I came up with for the GIF asset (it only plays once so you might want to open in a new tab and refresh a few times): It would be great to get some additional eyes on this — @pablohoneyhoney, does this asset look ok to you to use in a new Welcome Guide to introduce reusable blocks? This implementation is based on some of your previous notes from #29871. Please see the most recent video above in #32464 (comment) for context!
Thanks @Mamaduka! I was just looking at this Trac ticket and tried to match the asset as closely as possible to what was uploaded there. Looks like there is also a SVG static asset needed which I can supply as well pending any feedback :) |
Thanks, @critterverse. If you can provide assets similar to the ones in the Trac ticket, that would be perfect. @thisissandip it looks like we have ESLint errors. We should fix those to pass all the required checks. |
The lock/unlock PR will affect this PR, as one should not be able to save a change as long as the lock is closed/locked. |
Not necessarily. The "Save" button is disabled if there are no edits, and I think this will be the case when the block is "locked." |
Good point George! Let us get the lock/unlock PR in place first, so that we can also add some explorations in it before this PR merged. |
Adding in this comment especially: EDIT: NB! Both of the above comments needs to be addressed in the Locking PR as well as when clicking the Save button. Keeping the above comment in the back of my mind while I test this PR using this method: https://make.wordpress.org/design/2021/03/03/testing-a-gutenberg-pull-request-pr/ After activating the special Gutenberg plugin I received a lot of fatal errors, so that I had to manually remove the plugin from the plugin folder. It would be nice to get this fixed making it easier to test this PR. |
I'm not Pablo, but I like the animation. It could probably be a little smoother; There's some jitter in it now. Some easing could help a little. |
This is a subtle change but adding some easing definitely helps, thanks for the suggestion @shaunandrews! I also made a minor adjustment to the border radius on the page elements to better match the existing assets. Here's the updated version: @Mamaduka let me know when we have a Trac ticket and I'll add the SVG there, thanks again for your help with the assets 😁 I also had a few quick thoughts about some of the feedback we've been hearing around current confusion re: editing Reusable blocks (some of which was shared above by @paaljoachim). I think there are some small adjustments that we could include in this PR that might help:
|
I have updated the label to "Save Globally" and increased the margin of the save button since the button was not centered correctly within the Toolbar group as suggested by @critterverse |
@@ -18,7 +18,7 @@ | |||
// Styles for save button | |||
.components-toolbar-group { | |||
.block-library-block__reusable-block-save-button { | |||
margin: 7px; | |||
margin: 8px; |
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.
You can use $grid-unit-10 instead of 8px if you like.
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.
Oh, Yes! Thank you! 😄
Thank you for pointing this out @critterverse! I've updated the margin of the unsaved changes indicator in the parent selector button. Now it looks like this when focused: |
@thisissandip Sorry for not being very clear in my feedback above! To clarify, I think we could make an adjustment to the focus indicator versus adjusting the dot placement. Otherwise it causes the dot to jump around when moving between the toolbar and parent selector: Here's the spacing specs for easy reference: EDIT: Revised the specs below to match the current spacing used in this PR and deleted a comment referencing outdated spacing Re: Parent selector focus indicator — I wonder if we could resolve the issue with the dot being cut off by making the focusable area 2px wider? 🤔 Before adjustment: After adjustment: |
Hi @critterverse, thanks for the clarification!
Increasing the Parent selector focus indicator area by 1px solved the issue. |
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.
Thanks for your work here @thisissandip and for the iterations! Cool work! 💯
When I first saw the problem that you're trying to solve I thought that a custom hook would be good and then we could use that for other similar blocks, like Template Parts
maybe.. Unfortunately this is not so easy as we need a dependency of core-data
for hasEditsForEntityRecord
, unless there is another way I didn't think of.. 🤔
Regarding the slots
I'm a bit concerned creating two of them and so specific to the thing they add (unsaved changes indicator
). Do we expect something else to be added before the block icon
? Should we rename them to something more generic?
I don't have a clear proposal for these - just some thoughts of mine 😄
Also have you tried using only one slot? Could this work? And lastly without seeing all the css code, could some css added with :after
(or something similar) without adding a new div
node, could work and simplify things?
import { Guide } from '@wordpress/components'; | ||
import { __ } from '@wordpress/i18n'; | ||
|
||
function WelcomeGuideImage( { nonAnimatedSrc, animatedSrc } ) { |
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 have mixed feelings about this as well since we are not introducing a new concept here and save
button/action used to be there before.. Also I've tried your PR a bit yesterday and today and both times I loaded a post the first time, it showed me the basic welcome guide. I didn't investigate why this happened though. Did this happen for anyone else?
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.
isChildSelected: _isChildSelected, | ||
isSelected: _isSelected, | ||
}; | ||
}, [] ); |
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.
clientId
is a dependency here.
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.
Thank you! You saved me from future errors! 🚀
|
||
const parents = getBlockParents( selectedBlockClientId ); | ||
const firstParentClientId = parents[ parents.length - 1 ]; | ||
const _isChildSelected = firstParentClientId === clientId; |
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.
Shouldn't the case of changing nested blocks inside the reusable
block been handled somehow? I was expecting here to see a getBlockRootClientId
and check if is reusable
and hasEdits
... If we add a Group
with a Paragraph
inside the reusable block and change that paragraph, the block has edits but there is no indication for the changes. Of course in this case the Group
block is the parent.. I'm not sure how it would be best to show this.
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 expecting here to see a getBlockRootClientId and check if is reusable and hasEdits
Yes, this is much cleaner and better. Thank you for the suggestion. Updating this in my next commit.
If we add a Group with a Paragraph inside the reusable block and change that paragraph, the block has edits but there is no indication for the changes. Of course in this case the Group block is the parent.. I'm not sure how it would be best to show this.
Even I am not sure about this case cause one has to select the Reusable block itself to access the save button in the block toolbar.
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.
If we add a Group with a Paragraph inside the reusable block and change that paragraph, the block has edits but there is no indication for the changes.
Introducing dot indicators into List view has come up as one possible solution that would help with more visibility in instances with a lot of nesting. Needs a design explore but I could look at this alongside other follow ups in #33988!
@critterverse While I was investigating this, I found that the jump of the icon occurs on different screen widths. For example: Screen width 942px — icon jumps and again Screen width 1238px — icon jumps It seems the jump occurs only when the width of the screen size is an even number. This is a weird issue, I can't figure out the reason. (Maybe because of my monitor's low resolution?) Can you confirm if you can replicate this on your side? |
@ntsekouras Thank you for taking a look and for the code review! 😄
I gave this a shot and was able to implement this using only one slot and a single Unsaved Changed Indicator component called I have created a separate PR: thisissandip#2 in my forked repository to keep things clean. I will really appreciate it if you can take a look so maybe we can drop using 2 separate slots and 2 different components for the unsaved changed indicator.
Adding That's why I added a div for the dot (unsaved changes indicator) |
Oh weird! Yes, I can replicate...
FWIW I'm seeing the jump even when testing on a retina screen 🤔 |
|
||
// move the parent selector icon to right side to make room for the dot | ||
.block-editor-block-icon { | ||
margin-right: -15px; |
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.
Changing this value to -16px (or $grid-unit-20
) seems to get rid of the visual jump for me (tested on a couple of different screens at different viewport sizes).
This has sat for a bit but looks good from a UX perspective. Can we land this? |
Let's land this outside of WordPress 5.9, as it feels like one part of a bigger change. |
@thisissandip Is this something you're still interested in progressing? I realise the scope of this change increased dramatically, but it is a really valuable change, so if you're still interested that would be really great. If you don't have time or you have other commitments, would you be ok with another community member working on this? Ideally it'd be done in a way where you still get props for the majority of the work. |
// 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 | ||
parentSelectorBtn.classList.remove( 'parent-block-has-changes' ); | ||
contextualToolBar.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.
Something that I mentioned on the other PR (thisissandip#1), I think it'd be good to make this slot not modify the element's class names in this way. This couples the UnsavedChangesIndicator
to the class name of the parent selector button, if that classname changes then this breaks.
I recall it was because the parent selector button is positioned absolutely. That seems unusual to me, and like some tech debt. Maybe those toolbar styles can be refactored.
Hi @talldan 👋 ,
I am so sorry for not being able to update this PR as I am busy with some other commitments. It would be okay with me if someone else pick this up from here 🙂 I just want to make a note of the things that are left for this PR:
Once again, I am sorry for not being able to work on this PR at the moment. I'll come back to contribute to Gutenberg again in a few weeks! |
@kellychoffman added this fix in 740a109 so I think the only remaining issue is the one @talldan mentioned.
Totally understandable, @thisissandip. Thank you so much for all the great work on this PR and for seeing it through across so many iterations! 👏 |
The related issue has now been closed, so I'll close the PR as well. Thanks for all the explorations here. |
Fixes #32020
Description
Added a Save Button for Reusable Blocks in Blocks Toolbar. If there are no changes, the save button is disabled. When a change is made in that particular reusable block. One can save the changes from the block toolbar. Once saved, the user is notified with a snackbar about the save. The snackbar's 'Learn More' opens the Reusable Block Welcome Guide which also has a link to external documentation.
Since the image (gif) for the welcome guide is not ready yet. I have kept a plain background in its place.
I will add the image whenever it's made available.
How has this been tested?
Screenshots
ReusableBlockSaveButton.mov
Types of changes
Checklist: