-
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
Chrome: Enhance the publish button flow (and the saved state component) #945
Conversation
@youknowriad: have you seen #920, which allows us to know whether the last save request was for a new draft ( |
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. |
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. |
I totally love this. I feel like the experience completely vindicates the idea of separating the two. Thanks so much for working on this. 👍 |
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:
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]". |
Worth noting there's a difficult to solve edge case in this PR: We can't have
@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? |
@jasmussen I looked at Calypso, and here's how it's solved:
Does this sound ok for you? |
That sounds pretty good to me, yes. |
bb371a3
to
9392e26
Compare
@jasmussen It should be ok now. I'm using |
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? |
Good catch, it's fixed now by 1ab9d1e but I can't understand why the |
1ab9d1e
to
c85ac20
Compare
Expected: "Publish" button labeled "Publish" |
|
||
@keyframes move_background { | ||
from { | ||
background-position: 0 0; |
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.
Tabbing inconsistency.
editor/header/saved-state/index.js
Outdated
if ( isSaving ) { | ||
return ( | ||
<div className={ className }> | ||
Saving |
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.
Should localize
editor/header/saved-state/index.js
Outdated
if ( ! isNew && ! isDirty ) { | ||
return ( | ||
<div className={ className }> | ||
<Dashicon icon={ 'saved' } /> |
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.
Need for curly braces? <Dashicon icon="saved" />
editor/header/saved-state/index.js
Outdated
text = wp.i18n.__( 'Saved' ); | ||
if ( isSaving ) { | ||
return ( | ||
<div className={ className }> |
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.
Do we want this to be a block node, or is inline span
better?
( dispatch ) => ( { | ||
onSave( post, edits, blocks ) { | ||
dispatch( savePost( post.id, { | ||
content: wp.blocks.serialize( blocks ), |
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 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?
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.
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.
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.
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.
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 it's probably better to leave this for a separate PR. The change doesn't concern this PR only.
dirty: isEditedPostDirty( state ), | ||
isSaving: isSavingPost( state ), | ||
isPublished: isEditedPostAlreadyPublished( state ), | ||
isBeingScheduled: isEditedPostBeingScheduled( state, 'date' ), |
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.
There's no second argument accepted for isEditedPostBeingScheduled
editor/header/tools/style.scss
Outdated
right: 0; | ||
bottom: 0; | ||
content: ''; | ||
background-image: repeating-linear-gradient( -45deg, $blue-wordpress-700, $blue-wordpress-700 11px, $blue-wordpress 10px, $blue-wordpress 20px ); |
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.
Tabbing inconsistency. We need stylesheet linting 😄
@@ -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 ); |
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.
Should this occur in date/index.js
setupLocale
?
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 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.
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 it independent this way though, if we're assigning defaults to the global instance?
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'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).
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 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
editor/state.js
Outdated
@@ -34,6 +34,8 @@ export const editor = combineUndoableReducers( { | |||
...state, | |||
...action.edits, | |||
}; | |||
case 'REQUEST_POST_UPDATE': |
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.
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.
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 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.
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'll comment on #902
editor/header/tools/style.scss
Outdated
right: 0; | ||
bottom: 0; | ||
content: ''; | ||
background-image: repeating-linear-gradient( -45deg, $blue-wordpress-700, $blue-wordpress-700 11px, $blue-wordpress 10px, $blue-wordpress 20px ); |
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.
Still not quite tabbed enough 😅
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.
🙃
editor/header/saved-state/index.js
Outdated
return ( | ||
<span className={ className }> | ||
<Dashicon icon="saved" /> | ||
{ wp.i18n.__( 'Saved' ) } |
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.
Now that we're importing __
, should we be consistent with how we're calling it throughout the file?
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.
🙃
position: relative; | ||
|
||
// These styles overrides the disabled state with the button primary styles | ||
background: none !important; |
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.
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 ?
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.
Unfortunately we can't. These are the styles of WP and they are marked as important
:(
fd48920
to
97ce8fd
Compare
@aduth I was able to fix the timezone issue, by creating a custom WP timezone for these custom timezones. 5f0fa08 |
ffe519d
to
691ec08
Compare
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 we don't include isNew
in REQUEST_POST_UPDATE_SUCCESS
, the effect handler for navigating to apply ?post_id
is no longer triggered.
…post is published
cc96bf0
to
82a9708
Compare
The behavior is now similar to Calypso where we hide the pending toggle on published posts. Makes things simpler.
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. |
Actually, scratch that last comment. This branch already solves the responsive issues that are in master! |
Yeah, let's try to merge this today. I think I've addressed all the concerns. I'll wait for another look |
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 left some comments but I think this is in a good state to merge if we want to iterate in subsequent pull requests.
|
||
dispatch( { type: 'CLEAR_POST_EDITS' } ); |
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.
Question: Should we wait until after the save has completed to clear edits? What if the save fails?
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.
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.
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.
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)
} ) { | ||
const buttonEnabled = ! isRequesting; | ||
|
||
const buttonEnabled = ! isSaving && |
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.
Thinking some of this more complex logic warrants testing, or moving to selector e.g. isPostPublishable
.
editor/selectors.js
Outdated
* @param {Object} state Global application state | ||
* @return {Boolearn} Whether the post has been bublished | ||
*/ | ||
export function isEditedPostAlreadyPublished( state ) { |
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.
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.
showHint={ false } | ||
/> | ||
</div> | ||
{ ! isPublished && |
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.
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.
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.
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
Thanks for the review @aduth I've addressed the selector notes. I'm going to merge this.
Should we create separate issues for each of these points? |
🎉 |
@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. |
closes #945
Several things in this PR:
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.