-
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 basic 'dirty state' handling #610
Conversation
editor/state.js
Outdated
}, | ||
dirty( state = false, action ) { | ||
switch ( action.type ) { | ||
case 'REPLACE_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.
After a rebase, this should be changed to "RESET_BLOCKS" and "REPLACE_BLOCKS" should be moved down.
Maybe we shouldn't show anything when the post is dirty (instead of ! modified)? |
I like the idea of showing something here, at least until (unless) we have autosaving implemented, but I agree the |
dfc001f
to
572f156
Compare
Rebased per #610 (comment). I think there are 2 options for this PR:
I don't have a clear preference, I don't expect this to be too difficult to rebase either way. |
@jasmussen / @mtias: thoughts on the design here (what should we show when the post content is modified?) Note this is a very early version of dirty detection and that we likely won't have auto-save support for some time, so I think there is value in a clear "Modified" indicator. |
Maybe try "Unsaved changes"? In any case, I think we might want to merge this really then tune as we use it. There's no better crucible than trial. Also, you're up early! |
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.
Let's merge this 👍
editor/state.js
Outdated
case 'MOVE_BLOCK_UP': | ||
case 'REPLACE_BLOCKS': | ||
case 'REMOVE_BLOCK': | ||
case 'SWITCH_BLOCK_TYPE': |
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: the SWITCH_BLOCK_TYPE
has been removed
Removed here too.
Done. |
A nice future enhancement would be reverting
Expected: Not dirty |
This would likely require a deep comparison of serialized content and other fields. One issue we would run into currently is that there are a ton of cases where we parse content differently than we serialize the resulting data structure. Whitespace, blocks with extra attributes/tags, attribute order, ... |
I don't know that the comparison would need to occur on serialized content, or that any parsing or whitespace need be considered at all. If we treat the editable content as an array of block objects, it should be roughly: const isDirty = ! deepEquals(
[ { blockType: 'core/heading', nodeName: 'h2', content: [] } ], // Before
[ { blockType: 'core/heading', nodeName: 'h3', content: [] } ] // After
); There's other complications of course, but I don't think we need to maintain a string copy of the post's content in state at all. Rather, we need only generate it on demand when the save action is initiated or switching between Visual and Text modes. |
Sounds like a good idea, but it's not clear to me how we would generate the array structure you mentioned. Unless I'm missing something, I don't think it's already in state? |
For current state, this might just be |
Questions/concerns:
I chose a simple approach here because I wasn't sure how to address the above concerns; also because I think the worst thing a "dirty" indicator can do is to show that changes have been made when they really haven't. I'm reminded of some unfortunate Calypso issues that I tried and failed to solve once 😭 The case above (a user makes a change then manually reverses it) is a bit different: yes, the user did change post content, even though the result is the same. |
I think the comparison would occur outside of state, using state values (the current values of the post, compared against either the original values or a subset of edited properties). Performance could be a consideration, but if those values are only updated when a change truly occurs, we could memoize on the assumption that two strictly equal states would have the same outcome. Here's an idea of what it could look like: https://github.com/Automattic/wp-calypso/blob/88232a1/client/state/posts/selectors.js#L333-L363
Not really, unless the block is assigning a unique value when it shouldn't be (counter, |
This PR adds a first pass at a
state.blocks.dirty
state key and uses it to display "Saved" or "Modified" in the toolbar:No changes to content
Note, this is always the current state in
master
.Content is dirty
Notes
Spun off from #594 (review) - after that PR is merged, I expect this will be called
state.editor.dirty
instead, and we can update this state key to be aware of post save actions and use it to alter the Publish/Update button message as well.This uses the list of actions from #594 (comment) to update the
dirty
state key. One thing that's missing: we should probably update the dirty state as soon as some content is edited within a block, rather than after the focus is switched to another block.