-
Notifications
You must be signed in to change notification settings - Fork 1
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 custom save button #24
Conversation
…use as a customizable save button.
…r intercepting sidebar shows/hides, use for "Save" button
…tic interception function AND a dynamic callback function
…actions when the current toolbar is open
…tatus editing disabled by default, add a debug "Edit" button for now
… status to avoid other impossible in-between states
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 looks good to me, it's a big improvement over how it works right now.
There's one improvement I'm thinking we could make:
Could we show the publish button, and our special save button alongside each other if the publish guard functionality is off? That could serve as the basis for selectively showing it to certain users.
This doesn't have to happen in this PR, it can happen in a follow up one imo.
A limitation we should note:
The save draft button shows when a post is first opened, and I don't think there's a way to hide that afaik.
<PluginPostStatusInfo | ||
className={ `vip-workflow-extended-post-status vip-workflow-extended-post-status-${ status }` } | ||
> | ||
<h4>{ __( 'Extended Post Status', 'vip-workflow' ) }</h4> |
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 thinking that we be bold about this, and take this out entirely. So the custom save button becomes the best way to move a post through the various statuses that are supported.
We can follow up with the approve/reject flow as well as the ability to set what statuses require reviews and which ones don't. We do have to think about when a post needs to quickly move through the statuses, and maybe a filter might be better for that but we can do that later.
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 thinking that we be bold about this, and take this out entirely. So the custom save button becomes the best way to move a post through the various statuses that are supported.
I agree with this approach but it also makes me wonder if we should support the reverse direction as well? Perhaps only certain users like editors? I'm thinking that there would certainly be certain scenarios when an editor might might want to make a post back to a previous status. E.g.: An assigned
post no longer has an author working on it so the editor decides to move it back to pitch
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 thinking that we be bold about this, and take this out entirely.
There is one main problem I'm concerned with, and that's accidentally moving a status forward. An editor might hit the blue toolbar button expecting it to be a "Save", and then accidentally move it to a status it isn't ready for yet. If that happens, the only "fix" is to recreate the post, or just ignore the custom status. There's no way to clean up that error.
I don't have a great solution for that, but maybe we could make the UI more friendly for regular saves. Here's a two-step concept where we just have a generic "Save" button that displays options for promote/save draft after clicking:
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 like @hanifn's idea to have a permissions-based control, like maybe only showing the "Edit" button for manage_options
users or some other way to move back. However, we don't have any backend controls for only allowing some users to move statuses backward, only a guard against publishing. How about we leave the "Edit" button in for now and punt the work to allow permissions-based status change edits for another PR?
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.
My main concern was how confusing it could be to have both of those in there. I'm good with leaving it for now, and thinking about it as part of the approve/reject flow. We can potentially do the exact thing that you are suggesting since we do have a full sidebar UI for ourselves.
Could you add a short description to that extended status explaining this briefly? That way it's a touch less confusing.
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.
return null; | ||
} | ||
|
||
const customStatusName = VW_CUSTOM_STATUSES.status_terms.find( t => t.slug === status )?.name; |
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.
Minor: Would replace the t with term. Not a fan of one word variables unless it's a counter
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.
Fixed in 240034c. I usually also prefer longer variable names, but for one-liner lambdas I sometimes take the lazy route. Happy to stick with longer, though.
{ /* Custom save button in the toolbar */ } | ||
{ isCustomSaveButtonVisible && ( | ||
<PluginSidebar name={ sidebarName } title={ buttonText } icon={ InnerSaveButton }> | ||
{ /* Use this space to show approve/reject UI or other sidebar controls */ } |
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.
Would switch this to a //ToDo
structure. Trying to standardize so it's easy to find these ToDo entry points
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.
Fixed in 240034c.
interfaceStore.name, | ||
'enableComplementaryArea', | ||
( originalAction, args ) => { | ||
if ( args[ 0 ] === 'core' && args[ 1 ] === sidebarName ) { |
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.
Is this always going to be an arguments array of length 2 or could there be an out of bounds problem?
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.
Always good to be safe. Changed to:
vip-workflow-plugin/modules/custom-status/lib/hooks/use-intercept-plugin-sidebar.js
Line 14 in 047bcce
if ( args?.[ 0 ] === 'core' && args?.[ 1 ] === sidebarName ) { |
@ingeniumed I tried this out and I think it works really well: Screen.Recording.2024-08-29.at.2.13.36.PM.movWe still have a toolbar space issue that's even more noticeable with more buttons, but overall I think this is is a good middle-ground for understanding the publish guard feature while still allowing a jump straight to publishing. I'll add that in this PR. |
Love the addition of the new publish button alongside the save button, this looks good to me! |
Description
This PR adds a custom save button that can be used to guide a user through a set of custom statuses:
Screen.Recording.2024-08-28.at.12.52.51.PM.mov
This button currently moves posts through each custom post status, in-order, until "Publish" is available. However, we can extend the custom save button to:
Open a sidebar for additional actions, like an Approve/Reject box or leave a comment. For example:
Remove the button entirely when an action requires third-party interaction, like removing the "Publish" button for writers until an editor has approved the post.
Contain more complex logic, like moving a post back to a prior status if rejected.
For now, this simplifies the workflow status process and gives us access to a fully-customizable save button.
Debug controls
The "Extended Post Status" sidebar item currently has an "Edit" button that allows status switching for debugging purposes:
I'm not sure if we should keep this in, restrict it to higher-level users, or entirely remove it. I found it helpful for testing, so I've left it in for now.
Steps to Test
Ensure "Publish Guard" is enabled in Admin -> VIP Workflow -> Settings -> Publish Guard. The custom button is currently only activated when publish guard is turned on.
Create a new post. The "Save" button that is initially present on the page is the native button.
Click "Save". The native button should be replaced by our custom button showing "Move to
<next status>
".Click the button. The post status should change to the next status. Repeat clicking the button until the final custom status is reached.
On the final status, the custom button will again be replaced with the native button, showing "Publish". Clicking this should publish the post as usual.
Implementation
Overriding or customizing the save button is not supported by Gutenberg in the post editor. Some modifications are possible like filtering internationalization labels, but in general there is little customization of style or behavior.
We've attempted some other strategies for guiding a user through workflow steps (#15, #11, #9, #4), but we haven't had success leaning on the native save button. In this PR, we replace the native save button with a customizable button when custom statuses are enabled and in use.
Rendering a custom save button requires a few tricks:
Button rendering
The custom button is actually a
<PluginSidebar>
similar to those used by Jetpack or the Parse.ly plugin. In order to display text instead of an icon in the button contents, we pass anInnerSaveButton
in theicon
slot of the sidebar:vip-workflow-plugin/modules/custom-status/lib/custom-status-block.js
Line 87 in 29c5918
This inner button is just a
<div>
that holds some classes and the "Move to..." text:vip-workflow-plugin/modules/custom-status/lib/custom-status-block.js
Lines 147 to 155 in 29c5918
The final step to making this "icon" look like a save button is to apply some styling. We also use some CSS
has()
rules in order to change the button's style, like adding the saving animation to the outer button whenis-busy
is applied to the inner button text:Catching button clicks
Normally clicking a
<PluginSidebar>
button would cause a sidebar to open like this:We really just want this button to function as a regular button. Additionally, we want the ability to have the button open the sidebar sometimes, like if we need additional input from the user. There are a few steps to make that work:
First, the
useInterceptActionDispatch()
hook is used to catchdispatch()
ed events on a store. With this function, we can catch or modify a store's dispatch action.Next,
useInterceptPluginSidebar()
intercepts the interface store'senableComplementaryArea
anddisableComplementaryArea
dispatch functions. These control when the sidebar is opened and closed.These intercepts catch when our custom button is clicked and pass a callback function to open the sidebar if desired.
Lastly, the plugin calls useInterceptPluginSidebar() here. We provide a callback which is essentially an
onClick
handler for the custom save button, which also has the ability to open a custom sidebar by callingtoggleSidebar()
. In the current implementation we use this to move a post to the next workflow status.Hiding the native "Save" button
It's important that we do not completely override the native
<PostPublishButton>
as it contains complex logic for normal post save handling. Instead, we want to override the button only when a custom status is being used, and then delegate back to WordPress when a post is no longer in a custom status. We also want to do this reactively without needing to refresh the page.The main logic for when we show a custom button lives in
isCustomSaveButtonEnabled()
, which basically just checks if we're currently using a VIP Workflow custom status. When this evaluates to true, a custom class is added to the top-level editor:vip-workflow-plugin/modules/custom-status/lib/custom-status-block.js
Lines 38 to 47 in 29c5918
We use this class in CSS to hide the native button when present:
vip-workflow-plugin/modules/custom-status/lib/editor.scss
Lines 82 to 88 in 29c5918
The effect is that whenever
isCustomSaveButtonVisible
changes, the "Save" button is reactively added or removed from the screen.Dealing with small viewport sizes
The toolbar can get pretty crowded and cause elements to overlap. This is even true in the standard editor without customizations:
Overlapping buttons in the native editor toolbar
Ideally our custom button can be at least as good or better than the native experience when dealing with smaller viewports. I don't think we're there yet, but there are a couple of mitigations used in the custom button:
For one mitigation, we check for a wide viewport using Gutenberg's super nifty
useViewportMatch()
hook:vip-workflow-plugin/modules/custom-status/lib/custom-status-block.js
Line 29 in 29c5918
If it's not wide, we truncate the status text, which can be seen above:
vip-workflow-plugin/modules/custom-status/lib/custom-status-block.js
Lines 182 to 190 in 29c5918
A second mitigation is to grow the button to two lines when the editor is at roughly mobile size, also seen above. This works by checking for a
tiny
viewport, which is used to set theis-tiny
class on the button, and styled to allow wrapping an two line heights:vip-workflow-plugin/modules/custom-status/lib/editor.scss
Lines 73 to 79 in 29c5918
One other possible option would just be to change the button to say "Save" on tiny viewports, and on-click open the sidebar to show separate "Save Draft" and "Move to
<next status>
" buttons separately.The current implementation still isn't ideal. There are some visible overlaps, and the issue is made more difficult by custom status names that can be of variable length, plus the "Move to " text prepended to each one.
If you have any suggestion on ways we can tweak the current parameters or improve visibility on all viewports, please let me know!