Chrome: Enhance the publish button flow (and the saved state component) #945

youknowriad · 2017-05-30T17:20:02Z

closes #945

Several things in this PR:

We need a way to match the server's timezone in the frontend in order to compare dates properly. I'm using moment-timezone for this and setting the server timezone globally in moment
When publishing a post (publish button), the status applied to the button 'publish' if no date or date in the past, 'future' if date in the future but this status can be overridden when the edits (pending, private, ...)
When clicking the "save" link, we always save a draft unless the post is already saved, we'll keep the same status in this case.

I'd appreciate help in testing this PR because there's so many use-cases. It's easy to miss one.

Auto-saving is not implemented yet.

nylen · 2017-05-30T18:09:35Z

@youknowriad: have you seen #920, which allows us to know whether the last save request was for a new draft (Saved!) or an existing post (Updated!)? This is not quite working correctly in master. I see that the related requestIsNewPost property was instead removed in this PR, and I am wondering how to reconcile these two PRs.

youknowriad · 2017-05-30T18:11:48Z

I see that the related requestIsNewPost property was instead removed in this PR, and I am wondering

I didn't remove it because it was broken but because it's not needed anymore. The button doesn't need to know if the last request was for an unsaved post or not.

nylen · 2017-05-30T18:16:52Z

If we don't need it, let's also remove that part of the state tree and the related action properties. I wasn't very happy with how that looked anyway.

jasmussen · 2017-05-31T07:32:28Z

I totally love this. I feel like the experience completely vindicates the idea of separating the two. Thanks so much for working on this. 👍

jasmussen · 2017-05-31T07:41:27Z

Question: does auto-save ever kick in? If not, then how does that work, if at all? And if not, we should probably ticket this separately.

In the context of this design, I imagine the behavior being like this:

Start new post. Left side area is empty
Type a little. Left side shows "Save".
3a. Wait a few seconds without typing, left side shows "Saving..." then "✔ Saved"
3b. Or, don't wait, click Save manually, and left side shows "Saving..." then "✔ Saved"

Autosave can totally be addressed separately. Additional future enhancements, could be an autosave error message when you're trying to save but the internet is down. This error message should show up on the left, like "❗ Couldn't save [Try again]".

youknowriad · 2017-05-31T09:21:03Z

Worth noting there's a difficult to solve edge case in this PR: We can't have

a "pending" and "draft" post at the same time because this information is stored in the same "status" field. So when hitting "Save" on a post where we activated the "pending" toggle. This will be reset since the post is saved as "draft".

@jasmussen This PR doesn't address auto-saving yet. I was planning to do this separately if the workflow here is validated (probably needs a separate issue). I'm not sure how to solve the issue above when auto-saving (Leaving the pending toggle activated and saving the post at the same time). The solution will be to save all the user changes except the "status" but this leaves the post as "dirty" which means the "Saved !" message will never appear. I'm thinking of showing this message for like 5 seconds after the save is successful (whether it's auto-save or explicit) and don't take into consideration the "dirty" property. Thoughts on this?

Edit: We have the same issue with the "private" toggle.

jasmussen · 2017-05-31T09:27:03Z

@jasmussen This PR doesn't address auto-saving yet. I was planning to do this separately if the workflow here is validated (probably needs a separate issue). I'm not sure how to solve the issue above when auto-saving (Leaving the pending toggle activated and saving the post at the same time). The solution will be to save all the user changes except the "status" but this leaves the post as "dirty" which means the "Saved !" message will never appear. I'm thinking of showing this message for like 5 seconds after the save is successful (whether it's auto-save or explicit) and don't take into consideration the "dirty" property. Thoughts on this?

Edit: We have the same issue with the "private" toggle.

@mtias @timmyc can you recall how we solved this in Calypso?

youknowriad · 2017-05-31T09:38:26Z

@jasmussen I looked at Calypso, and here's how it's solved:

For private posts, toggling the "private" status publishes privately the post directly without another click on the "publish" button. This solves the issue, because the auto-save will just save the published post again without any status change.
For pending posts, toggling the "pending" toggle don't save the post immediately but auto-saving and saving the post manually save the post as "pending" instead of "draft".

Does this sound ok for you?

jasmussen · 2017-05-31T09:39:11Z

That sounds pretty good to me, yes.

youknowriad · 2017-05-31T10:48:35Z

@jasmussen It should be ok now. I'm using window.confirm temporary instead of a Modal (we don't have that yet). I think we should ticket the Modal and the auto-saving separately. This PR is big enough, isn't it?

jasmussen · 2017-05-31T11:08:13Z

Nice, this is working and looking really well.

Visibility controls seem to have stopped working for me, though. Think it's this branch, or the webpack deduplication stuff?

youknowriad · 2017-05-31T11:18:47Z

Visibility controls seem to have stopped working for me, though. Think it's this branch or the webpack deduplication stuff?

Good catch, it's fixed now by 1ab9d1e but I can't understand why the onChange handler is not enough. Really weird.

aduth · 2017-06-01T14:17:16Z

Navigate to Gutenberg
Click Save

Expected: "Publish" button labeled "Publish"
Actual: "Publish" button labeled "Schedule"

aduth · 2017-06-01T14:09:04Z

editor/assets/stylesheets/_animations.scss

+
+@keyframes move_background {
+	from {
+    background-position: 0 0;


Tabbing inconsistency.

aduth · 2017-06-01T14:17:38Z

editor/header/saved-state/index.js

+	if ( isSaving ) {
+		return (
+			<div className={ className }>
+				Saving


Should localize

aduth · 2017-06-01T14:18:12Z

editor/header/saved-state/index.js

+	if ( ! isNew && ! isDirty ) {
+		return (
+			<div className={ className }>
+				<Dashicon icon={ 'saved' } />


Need for curly braces? <Dashicon icon="saved" />

aduth · 2017-06-01T14:18:58Z

editor/header/saved-state/index.js

-		text = wp.i18n.__( 'Saved' );
+	if ( isSaving ) {
+		return (
+			<div className={ className }>


Do we want this to be a block node, or is inline span better?

aduth · 2017-06-01T14:19:47Z

editor/header/saved-state/index.js

+	( dispatch ) => ( {
+		onSave( post, edits, blocks ) {
+			dispatch( savePost( post.id, {
+				content: wp.blocks.serialize( blocks ),


I wonder if block serialization should occur as part of a middleware before save proceeds into the network request handler, instead of the responsibility of the save button. What do you think?

maybe. To be honest, sometimes I think we should avoid passing any edits, or content, or blocks here and use getState instead inside the effect handler. because it feels like I'm adding extra props to components when the component doesn't really need those props.

because it feels like I'm adding extra props to components when the component doesn't really need those props.

Yeah, that's more-or-less how I feel too. It doesn't seem to follow that the save button ought to be concerned with how to create the payload for the request. I think either an effect handler or a separate standalone middleware could serve in its place. I mentioned middleware only because I'm curious to see how far we can take effects to not rely on the store argument, per some of my own goals outlined in the refx 2.0 changelog. I'd be fine with either approach, honestly, and the effect infrastructure being in place is a convenient lure.

I think it's probably better to leave this for a separate PR. The change doesn't concern this PR only.

aduth · 2017-06-01T14:20:41Z

editor/header/tools/publish-button.js

+		dirty: isEditedPostDirty( state ),
+		isSaving: isSavingPost( state ),
+		isPublished: isEditedPostAlreadyPublished( state ),
+		isBeingScheduled: isEditedPostBeingScheduled( state, 'date' ),


There's no second argument accepted for isEditedPostBeingScheduled

aduth · 2017-06-01T14:20:51Z

editor/header/tools/style.scss

+		right: 0;
+		bottom: 0;
+		content: '';
+    background-image: repeating-linear-gradient( -45deg, $blue-wordpress-700, $blue-wordpress-700 11px, $blue-wordpress 10px, $blue-wordpress 20px );


Tabbing inconsistency. We need stylesheet linting 😄

aduth · 2017-06-01T14:22:02Z

editor/index.js

@@ -12,6 +19,10 @@ import './assets/stylesheets/main.scss';
 import Layout from './layout';
 import { createReduxStore } from './state';

+// Configure moment globally
+moment.locale( settings.l10n.locale );


Should this occur in date/index.js setupLocale ?

The question is: if we do this in data/index.js, wp-data won't be library independent anymore. If you look at the code, you'll notice it's already done there, temporary but reverted to the original locale.

Is it independent this way though, if we're assigning defaults to the global instance?

It's independent in wp-date but an app like Gutenberg can make it global explicitly. That's my thinking. But I don't mind moving this to wp-date but we'd have to acknowledge that wp-date is moment dependent (externally).

I don't think we necessarily need to solve this here. Proposed for more discussion on the related Trac ticket: https://core.trac.wordpress.org/ticket/39222#comment:35

aduth · 2017-06-01T14:23:42Z

editor/state.js

@@ -34,6 +34,8 @@ export const editor = combineUndoableReducers( {
 					...state,
 					...action.edits,
 				};
+			case 'REQUEST_POST_UPDATE':


Do you think it be more semantic for edits to clear themselves by a standalone action CLEAR_POST_EDITS, which the effect handler(s) for REQUEST_POST_UPDATE would be responsible for returning/dispatching? Still unsure just how much we want to tie editor state to network activity, especially now that we have patterns for relatively simple effect chains.

I understand this concern. And this is part of defining what is our Data Approach, how do we handle async actions etc... We should document this and be consistent.

I'll comment on #902

aduth · 2017-06-01T15:37:47Z

editor/header/tools/style.scss

+		right: 0;
+		bottom: 0;
+		content: '';
+  	background-image: repeating-linear-gradient( -45deg, $blue-wordpress-700, $blue-wordpress-700 11px, $blue-wordpress 10px, $blue-wordpress 20px );


Still not quite tabbed enough 😅

aduth · 2017-06-01T15:38:15Z

editor/header/saved-state/index.js

+		return (
+			<span className={ className }>
+				<Dashicon icon="saved" />
+				{ wp.i18n.__( 'Saved' ) }


Now that we're importing __, should we be consistent with how we're calling it throughout the file?

aduth · 2017-06-01T15:51:14Z

editor/header/tools/style.scss

+	position: relative;
+
+	// These styles overrides the disabled state with the button primary styles
+	background: none !important;


Can we override by exceeding specificity of those default styles, instead of through !important ? Why are we breaking consistency with default disabled button styling ? Should we be applying these overrides to Button more generally if they're warranted ?

Unfortunately we can't. These are the styles of WP and they are marked as important :(

youknowriad · 2017-06-02T13:04:35Z

@aduth I was able to fix the timezone issue, by creating a custom WP timezone for these custom timezones. 5f0fa08

aduth

Since we don't include isNew in REQUEST_POST_UPDATE_SUCCESS, the effect handler for navigating to apply ?post_id is no longer triggered.

…e status

…post is published

The behavior is now similar to Calypso where we hide the pending toggle on published posts. Makes things simpler.

jasmussen · 2017-06-07T07:57:41Z

Excited about this branch! I'd like to do some responsive work. Should I do that in this branch, or wait until it's merged into master? I'm reluctant to do it in master currently, awaiting this one being merged in.

jasmussen · 2017-06-07T08:27:23Z

Actually, scratch that last comment. This branch already solves the responsive issues that are in master!

youknowriad · 2017-06-07T08:53:13Z

Yeah, let's try to merge this today. I think I've addressed all the concerns. I'll wait for another look

aduth

I left some comments but I think this is in a good state to merge if we want to iterate in subsequent pull requests.

aduth · 2017-06-07T12:08:10Z

editor/effects.js


+		dispatch( { type: 'CLEAR_POST_EDITS' } );


Question: Should we wait until after the save has completed to clear edits? What if the save fails?

In fact, I notice something odd about this behavior. When changing visibility for a published post and Update-ing the post, the visibility reverts back to its original value briefly. I suspect it's because of this dispatch here.

Hard problem to solve, we should probably diff the edits before and after save to avoid loosing the unsaved edits (if we move this call in the onSuccess)

aduth · 2017-06-07T12:09:45Z

editor/header/tools/publish-button.js

 } ) {
-	const buttonEnabled = ! isRequesting;
-
+	const buttonEnabled = ! isSaving &&


Thinking some of this more complex logic warrants testing, or moving to selector e.g. isPostPublishable.

aduth · 2017-06-07T12:11:34Z

editor/selectors.js

+ * @param  {Object}   state Global application state
+ * @return {Boolearn}       Whether the post has been bublished
+ */
+export function isEditedPostAlreadyPublished( state ) {


Question: Would this make more or less sense without "Already". I think it helps highlight that it tests whether the post was scheduled at a date before the current time, but maybe too much so to the extent that it might not be intuitive to use for a simple publish test.

aduth · 2017-06-07T12:12:36Z

editor/sidebar/post-status/index.js

-						showHint={ false }
-					/>
-				</div>
+				{ ! isPublished &&


What would it take to be able to show this for published posts as well? I'd suppose we need to revert back to the post's original status when toggling off, not a static value. Maybe an action that clears the current edited value for a property (we might want that for what we have anyways).

I'm not sure it makes much sense to revert from Published to Pending Review, but it is possible in the current editor.

Maybe if we at least have flow for reverting to draft it becomes a little more reasonable, since there's a workflow for changing from Published to Pending Review.

Good point, enabling it is not difficult to achieve, but yeah, it doesn't make much sense. I'm more in favour of having a "revert to draft" button here. cc @jasmussen

… the publish button

youknowriad · 2017-06-07T16:40:26Z

Thanks for the review @aduth I've addressed the selector notes. I'm going to merge this.
Here's a summary of the points to be addressed separately:

"Revert to draft" Button
Enhance the save effect to retrieve the edits and blocks from the store (as opposed to being passed as an argument)
Enhance the save effect to reset Edits on Save Success (and handle the edits made while saving)

Should we create separate issues for each of these points?

jasmussen · 2017-06-07T16:54:25Z

🎉

aduth · 2017-06-07T17:08:10Z

"Revert to draft" Button

@jasmussen Do we have this in any of the other mockups?

@youknowriad Yeah, we should create separate issues. I'm hoping to take a look at a "save" middleware (re: second point) soon.

jasmussen · 2017-06-07T17:12:37Z

Do we have this in any of the other mockups?

No specific mockups, no, but when we implemented this design we did for core, on WordPress.com first, we also designed that button there. Here's how it looks:

youknowriad added the General Interface Parts of the UI which don't fall neatly under other labels. label May 30, 2017

youknowriad self-assigned this May 30, 2017

youknowriad requested review from nylen and jasmussen May 30, 2017 17:20

youknowriad force-pushed the update/publish-button branch from bb371a3 to 9392e26 Compare May 31, 2017 10:28

youknowriad force-pushed the update/publish-button branch from 1ab9d1e to c85ac20 Compare May 31, 2017 11:20

nylen mentioned this pull request May 31, 2017

Preserve isNew after save completes successfully #920

Closed

youknowriad requested a review from aduth June 1, 2017 10:46

aduth reviewed Jun 1, 2017

View reviewed changes

youknowriad force-pushed the update/publish-button branch from fd48920 to 97ce8fd Compare June 1, 2017 16:11

youknowriad force-pushed the update/publish-button branch 2 times, most recently from ffe519d to 691ec08 Compare June 2, 2017 13:22

aduth previously requested changes Jun 2, 2017

View reviewed changes

youknowriad added 10 commits June 6, 2017 10:24

Chrome: Saving post edge cases, privately publish and saving keeps th…

8d24f6e

…e status

Chrome: Fix styles after rebase

28e0588

Chrome: Drop unused isNew flag on the saving request

56554d4

Build: Fix panel unit tests

a5c330d

Chrome: Fix visibility toggles

95ac35a

Chrome: Changes per review for the Publish Button

7ce6814

Date: Handling custom WP timezones

bf4ebbe

Chrome: Drop useless variable after rebase

2e3cdd6

Chrome: Fix redirection when saving new posts

84730ac

Chrome: Fix publish button flow and hide the pending toggle when the …

82a9708

…post is published

youknowriad force-pushed the update/publish-button branch from cc96bf0 to 82a9708 Compare June 6, 2017 09:24

jasmussen mentioned this pull request Jun 6, 2017

Polish responsive a bit #1035

Merged

Chrome: Fix double puvlish privately warning

f49e9b5

aduth reviewed Jun 7, 2017

View reviewed changes

Chrome: Adding the isEditedPostPublishable selector to enable/disable…

11aabf0

… the publish button

youknowriad merged commit 73bd4ab into master Jun 7, 2017

youknowriad deleted the update/publish-button branch June 7, 2017 16:47

youknowriad mentioned this pull request Jun 7, 2017

UI: Separate out drafting, saving, publishing actions #707

Closed

aduth mentioned this pull request Jun 7, 2017

Generate edit payload in save effect handler #1062

Merged

youknowriad mentioned this pull request Jun 9, 2017

Chrome: Clear Post edits on save success #1092

Closed

aduth mentioned this pull request Sep 6, 2017

Make Pending review toggle less prominent #1726

Closed

snyk-bot mentioned this pull request Jan 22, 2020

[Snyk] Fix for 1 vulnerabilities SeppPenner/gutenberg#113

Merged

Chrome: Enhance the publish button flow (and the saved state component) #945

Chrome: Enhance the publish button flow (and the saved state component) #945

Conversation

youknowriad commented May 30, 2017 • edited Loading

nylen commented May 30, 2017

youknowriad commented May 30, 2017

nylen commented May 30, 2017

jasmussen commented May 31, 2017

jasmussen commented May 31, 2017

youknowriad commented May 31, 2017 • edited Loading

jasmussen commented May 31, 2017

youknowriad commented May 31, 2017

jasmussen commented May 31, 2017

youknowriad commented May 31, 2017

jasmussen commented May 31, 2017

youknowriad commented May 31, 2017

aduth commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jun 2, 2017 • edited Loading

aduth left a comment

Choose a reason for hiding this comment

jasmussen commented Jun 7, 2017

jasmussen commented Jun 7, 2017

youknowriad commented Jun 7, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jun 7, 2017

jasmussen commented Jun 7, 2017

aduth commented Jun 7, 2017

jasmussen commented Jun 7, 2017

youknowriad commented May 30, 2017 •

edited

Loading

youknowriad commented May 31, 2017 •

edited

Loading

youknowriad commented Jun 2, 2017 •

edited

Loading