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

Perform dirty detection as diff against saved post #1996

Merged
merged 2 commits into from
Aug 8, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 24, 2017

Closes #916

This pull request seeks to refactor dirty detection not as a property of state, but instead as a selector whose return value is determined by a difference between state and the last saved copy of the post.

Implementation notes:

This one turned out to be more of a sprawling refactor than I would have liked, particularly around:

  • Flattening rendered post attributes into their raw value
  • Changing editor.edits reducer to omit values matching the reset post
  • Modifying TextEditor to operate without an explicit markDirty action creator

It may be worth splitting these into smaller pull requests.

Some remaining tasks include:

  • Verify edits made while save in-flight are respected
  • Update unit tests for editor/tests/state.js

Testing instructions:

Verify that the "Save" option is only presented when changes exist to be saved. Notably:

  • Shown on the Demo screen by default
  • Shown when changing content in Visual and Text modes
  • Shown when changing attributes (e.g. title)
  • NOT Shown when changing content or attributes, then reverting back to the original value

Demonstration of the last item:

dirty

Caveats:

Editable values are only reflected in state after blurring the editable fields. Dirtiness detection will only update at these intervals.

@aduth aduth added Framework Issues related to broader framework topics, especially as it relates to javascript [Status] In Progress Tracking issues with work in progress labels Jul 24, 2017
@nylen
Copy link
Member

nylen commented Jul 26, 2017

As previously discussed, this makes me a bit nervous because we are moving away from a pretty simple and predictable approach based on whether or not the user takes specific actions to something more like what we have in Calypso, and this has problems: Automattic/wp-calypso#9833 and others.

Undoubtedly there are a lot of differences between Calypso and Gutenberg, one being that we have no lingering history of using Flux as a data store. Still, if this has the potential to cause difficult-to-diagnose problems later on, then I don't think it's worth it.

What do you think? How can we avoid repeating the same bugs that exist in the Calypso editor here?

@aduth
Copy link
Member Author

aduth commented Jul 26, 2017

The current situation is certainly not much better, in that we prompt the user that there are changes when there may be none at all, only because they performed some action that triggered the dirty flag, or some rogue action dispatch flipped it into the dirty state.

Historical issues with dirtiness in Calypso can be attributed partly to its handling of content in two separate formats: "HTML content" and "raw content". To some extent we still have that here, only in that editing content in Text mode needs its value represented as a string. In all other cases, we can work with blocks as raw objects, which should hopefully be less unpredictable.

Undo could play a role here. If I come to rely on it for reversing changes, it could be a non-issue. Depending on if I as a user...

  • Change title, realize I made a mistake, then backspace to original title.
    • Result: I am frustrated because the page prompts me when I leave, even though I know that I reversed the title back to its original value
  • Change title, realize I made a mistake, press undo or Cmd/Ctrl+Z.
    • Result: All is okay because Undo reset post to a state where it doesn't consider itself dirty

I'd say end-to-end tests could help surface these problems, but then again, the prompts in Calypso have in many cases been browser-specific (IE11 and Firefox off the top of my head).

@aduth aduth force-pushed the update/diff-attributes-dirty branch from 01f5a4c to 810988b Compare August 4, 2017 19:30
@codecov
Copy link

codecov bot commented Aug 4, 2017

Codecov Report

Merging #1996 into master will increase coverage by 0.31%.
The diff coverage is 52.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1996      +/-   ##
==========================================
+ Coverage   24.06%   24.37%   +0.31%     
==========================================
  Files         149      150       +1     
  Lines        4592     4615      +23     
  Branches      775      783       +8     
==========================================
+ Hits         1105     1125      +20     
- Misses       2946     2947       +1     
- Partials      541      543       +2
Impacted Files Coverage Δ
editor/index.js 0% <ø> (ø) ⬆️
editor/modes/text-editor/index.js 0% <0%> (ø) ⬆️
editor/effects.js 29.68% <0%> (+2.92%) ⬆️
editor/actions.js 37.93% <0%> (ø) ⬆️
editor/layout/index.js 0% <0%> (ø) ⬆️
editor/autosave-monitor/index.js 50% <50%> (ø)
editor/state.js 88.27% <52.63%> (-5.35%) ⬇️
editor/selectors.js 96.24% <95.65%> (-0.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee1083a...da5cbe7. Read the comment docs.

@nylen
Copy link
Member

nylen commented Aug 7, 2017

@aduth I don't have strong opinions about this one other than the vague concerns noted above. If you think this is the best way forward then let's do it.

@aduth aduth force-pushed the update/diff-attributes-dirty branch from 810988b to abc0b3d Compare August 7, 2017 20:56
@aduth
Copy link
Member Author

aduth commented Aug 7, 2017

This is a tricky piece to get correct, and I have some reservations about the proposed implementation (dealing with editor history, juggling text view value). That said, I do think it's in our interest to try to make it work.

I've rebased and made a few revisions: Canceling pending autosave on <AutosaveMonitor /> unmount, unit tests for <AutosaveMonitor />, clearer variables and comments in isEditedPostDirty selector, test case for state.edits's handling of RESET_BLOCKS.

I'm gonna give this another thorough round of testing in the morning and merge if all looks good. At least being early in the week gives us some breathing room to resolve potential issues 😉

@aduth aduth removed the [Status] In Progress Tracking issues with work in progress label Aug 8, 2017
Can be difficult to adhere when interdependant functions in same file. Not error-prone.
@aduth aduth force-pushed the update/diff-attributes-dirty branch from abc0b3d to da5cbe7 Compare August 8, 2017 15:18
@aduth aduth merged commit acf4297 into master Aug 8, 2017
@aduth aduth deleted the update/diff-attributes-dirty branch August 8, 2017 15:28
Tug pushed a commit that referenced this pull request Mar 19, 2020
…use_polyfill

Update project to use react-native-url-polyfill package for the URL class
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants