Skip to content
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

Merged
merged 3 commits into from
May 4, 2017
Merged

Add basic 'dirty state' handling #610

merged 3 commits into from
May 4, 2017

Conversation

nylen
Copy link
Member

@nylen nylen commented May 2, 2017

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.

editor/state.js Outdated
},
dirty( state = false, action ) {
switch ( action.type ) {
case 'REPLACE_BLOCKS':
Copy link
Contributor

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.

@youknowriad
Copy link
Contributor

Maybe we shouldn't show anything when the post is dirty (instead of ! modified)?

@mtias mtias added Design General Interface Parts of the UI which don't fall neatly under other labels. Framework Issues related to broader framework topics, especially as it relates to javascript labels May 3, 2017
@nylen
Copy link
Member Author

nylen commented May 3, 2017

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 ! icon is a bit strong.

@nylen nylen force-pushed the add/content-dirty-state branch from dfc001f to 572f156 Compare May 4, 2017 05:57
@nylen
Copy link
Member Author

nylen commented May 4, 2017

Rebased per #610 (comment).

I think there are 2 options for this PR:

  1. Merge as-is, or with some design tweaks, then open separate PR(s) to integrate the status of the current post (whether it's a new or existing post; whether any other kinds of edits have been made).
  2. Merge Allow saving and publishing posts #594 first, then rebase this PR and make the logic here aware of save/update actions.

I don't have a clear preference, I don't expect this to be too difficult to rebase either way.

@nylen
Copy link
Member Author

nylen commented May 4, 2017

@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.

@jasmussen
Copy link
Contributor

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!

Copy link
Contributor

@youknowriad youknowriad left a 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':
Copy link
Contributor

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

@nylen nylen merged commit d85e20c into master May 4, 2017
@nylen nylen deleted the add/content-dirty-state branch May 4, 2017 10:55
@nylen
Copy link
Member Author

nylen commented May 4, 2017

the SWITCH_BLOCK_TYPE has been removed

Removed here too.

Maybe try "Unsaved changes"?

Done.

@aduth
Copy link
Member

aduth commented May 4, 2017

A nice future enhancement would be reverting dirty back to false when the user manually un-does their changes:

  1. Click heading block
  2. Change level
  3. Change level back to original value

Expected: Not dirty
Actual: Dirty

@nylen
Copy link
Member Author

nylen commented May 4, 2017

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, ...

@aduth
Copy link
Member

aduth commented May 4, 2017

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.

@nylen
Copy link
Member Author

nylen commented May 4, 2017

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?

@aduth
Copy link
Member

aduth commented May 4, 2017

For current state, this might just be deepEquals( state.blocks.byUid, originalState.blocks.byUid )

@nylen
Copy link
Member Author

nylen commented May 4, 2017

Questions/concerns:

  • Do we end up calling this method on every re-render? Is this a performance concern, especially with longer posts? If we're not calling this on re-render, how/where do we store it inside state? (Reducers can't/shouldn't depend on other parts of state)
  • I don't understand the inside of the blocks' content properties well enough to be confident that this will work. Can you think of a way it would break (saying that content is dirty when really no user action has been taken?)

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.

@aduth
Copy link
Member

aduth commented May 5, 2017

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

Can you think of a way it would break (saying that content is dirty when really no user action has been taken?)

Not really, unless the block is assigning a unique value when it shouldn't be (counter, Date.now()). There might be some challenges around comparing two rich virtual trees (React element Editable values), but I think this is solveable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants