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

Aztec: match undo and discard local changes options #8011

Merged
merged 4 commits into from
Jul 10, 2018

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Jul 10, 2018

Fixes #8010

From the point of view of the Editor, there are basically 2 moments where a Post can have a "Local changes" status:

a. either the Post already has that status when it's opened in the Editor, or
b. the Editor sets the isLocallyChanged flag on the Post object as a result of the user making changes to the title or content (or for the case, any of the Post's configuration settings).

This PR takes these two situations into account by making the Discard local changes appear either when there's some local history available (noting that the "change history" for now starts only when the Post is opened in the Editor), or when the isLocallyChanged flag was already set at the time this Post was opened in the Editor.

To test:

CASE A: post already had local changes.

  1. Open a Post that has Local changes flag in the Posts list (to make this happen, you can start a new draft, enter some text, turn airplane mode ON, and tap BACK - remember to turn airplane mode OFF then).
  2. check the overflow menu (tap the three dots icon at the top-right corner of the screen) shows the Discard Local Changes option

CASE B: post doesn't already have local changes.

  1. Open an existing Post
  2. check the menu does not show the Discard Local Changes option
  3. enter some text (a character is fine), effectively modifying the original content
  4. check the menu now does show the Discard Local Changes option
  5. tap on the Undo option - if you entered more text in step 3 just make sure there's nothing left to be undone by tapping undo until it's greyed out.
  6. once the undo option is not tappable anymore, obseve the Discard Local Changes option is not shown anymore.

cc @SylvesterWilmott following up from #8005 (comment)

@mzorz mzorz requested a review from theck13 July 10, 2018 15:07
@mzorz mzorz added this to the 10.5 milestone Jul 10, 2018
Copy link
Contributor

@theck13 theck13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this odd case related to discarding local changes!

I only have one very minor comment. If you think mOriginalPostHadLocalChangesOnOpen is better with its current name, I'm fine with merging this as is.

@@ -505,6 +506,7 @@ public void run() {
private void initializePostObject() {
if (mPost != null) {
mOriginalPost = mPost.clone();
mOriginalPostHadLocalChangesOnOpen = mOriginalPost.isLocallyChanged();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about naming mOriginalPostHadLocalChangesOnOpen something like mOriginalPostWasLocallyChanged since its only assignment is mOriginalPost.isLocallyChanged()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion @theck13 ! I think the current name, while a bit lengthier than the suggested one, better describes the specific situation where it's going to be used - that is, the value it's carrying is only meaningful OnOpen. We could either leave it as is or maybe rename it to mOriginalPostWasLocallyChangedOnOpen if you want? wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a big deal so we can leave it as is.

Copy link
Contributor

@theck13 theck13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@@ -505,6 +506,7 @@ public void run() {
private void initializePostObject() {
if (mPost != null) {
mOriginalPost = mPost.clone();
mOriginalPostHadLocalChangesOnOpen = mOriginalPost.isLocallyChanged();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a big deal so we can leave it as is.

@theck13 theck13 merged commit 9a37a6d into develop Jul 10, 2018
@theck13 theck13 deleted the try/check-undo-to-show-discard branch July 10, 2018 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants