-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Post change conflict resolution dialog #8989
Post change conflict resolution dialog #8989
Conversation
…on click for conflict resolution dialog
Can we review the copy? As a user "Load from remote" doesn't make a lot of sense to me. Maybe loop in editorial? |
Yes @elibud, this is what I wrote in the PR description, got probably lost 😄 :
I'd first wait for the functionality and UX to be agreed upon, then bring Editorial in if needed 👍 |
The terminology seems a little confusing here. A couple suggestions, and we should get copy review as well:
// cc @megsfulton for any additions or rebuttals :) |
Sorry about that, I should read carefully before commenting @mzorz 🤦♀️ |
For the status label I like what @iamthomasbishop suggested. For the dialog copy, I think we need to provide more information so the user can make an informed choice on which version to keep. Also we need to make it clear that only one version will continue to exist so the choice they're making which version to "keep." Resolve version sync conflict Local Cloud Keep Local Keep Cloud Then depending on which version they keep, can we have the editor open with the version they chose? |
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.
@mzorz I have only done a tentative check of this PR for now to keep things moving. I'll do a full review once we settle on a decision in the FluxC PR. I think everything looks right though, so I don't expect to add any other comments for the existing changes :)
onNegativeClickedForBasicDialog(instanceTag) | ||
// Cancel and outside touch dismiss works the same way for all, except for conflict resolution dialog, | ||
// for which tapping outside and actively tapping the "edit local" have different meanings | ||
if (instanceTag != CONFIRM_ON_CONFLICT_LOAD_REMOTE_POST_DIALOG_TAG) { |
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 guess this is mostly a design question, but would we prefer to make this a non-cancelable dialog instead? I feel like this is a good opportunity to make sure this conflict is resolved by the user. cc @megsfulton
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.
My thoughts are that when put under this situation, the user may also prefer to double check on the web before making a decision? Especially given we won't be (for now) allowing them to preview what's on one version and the other. So, forcing them to not have a way to escape would then let them stuck there until they can check on the web. Wdyt?
localPostIdForConflictResolutionDialog = null | ||
} | ||
|
||
private fun loadPostFromLocal(localPostId: Int) { |
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.
Again, this might be a design question, but when the user selects to edit the local post, shouldn't we push the current post to remote so the conflict is resolved? cc @megsfulton
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.
technically that's right, but just to add to this (and maybe making it less complex), if they choose to edit the local copy, it'll be sent to the remote side as soon as they exit the editor, so in the end it's probably the same result.
I love the recommendations on dialog copy, @megsfulton! I'm not sure how I feel about "cloud" but it's definitely more approachable than "remote". |
…elds for conflict status
… adjusted strings
…astModified() in PostUtils
Thanks for the input @megsfulton ! Regarding the dialog, can we change the wording from: Also using the basic Dialog I cannot use bold in within the message - we can totally do that with a custom Dialog but implies a bit more of effort, so I wanted to run it through you first and see if making a custom Dialog is worth that effort: How does that look to you? Also note I'm re-using an old function we had that uses the |
…PostListViewModel.kt Co-Authored-By: mzorz <mariozorz@gmail.com>
hi @oguzkocer thanks for the thorough review and the tips here and there! I think I've addressed all your comments now - after going ahead with each one I had to bring the nullifying statements back (the ones we got rid of / changed in 9494851, 9a38333, and b3bcec0) as they were before, did it in fd320bc. They were there for a purpose: coordinating the snackbars (in this sense this check here is of importance.), otherwise this was the output: As opposed to what can be seen in #8989 (comment) I understand it may not be as clear by looking into the code, I can introduce a new variable to make it explicit if not. Wdyt? Other than that, this should be ready to go - let me know your thoughts 🙇 |
…8989 Suggestions for the Post Conflict Resolution PR: 8989
…ange-conflict-resolution
@mzorz I took the liberty to merge |
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 turns out the auto-merge re-added the Introduced new site creation wizard
to the release notes which is definitely what we want. I've removed this change and since it doesn't affect the code, I am going to approve and merge this PR once it's done.
P.S: I'd normally work with the author on this, but the code freeze is early tomorrow (for me) and I won't have a chance to do that. These are very simple changes, so I am taking a chance here that it's OK. Let me know if you feel otherwise @mzorz and feel free to address them in a PR. Thanks!
Thank you @oguzkocer! 👍 |
This is a follow up to PR #8940
Comes from #5984 (comment)
After implementing #8940 as a fix to #8932, posts with local changes are not updated with the remote, and there is no indication if the remote had changes as well.
Quoting @megsfulton here for completeness:
What this PR does is:
Conflicted
for now).Conflicted
status, it shows a dialog asking whether they would like to load and the Post from the remote, or edit the local copy. They can cancel and not take any action by dismissing the dialog (either tap back arrow key or tap on the screen outside the dialog area)Corresponding FluxC PR wordpress-mobile/WordPress-FluxC-Android#1077
Note: this needs copy review, the text and approach here does not intend to be final but to show what the functionality would feel like. Also, the new status's color and icon should probably be different than now? cc @megsfulton .
To test:
CASE A: (show the new status on the Posts list)
Dialog shown here:
![screen shot 2019-01-16 at 12 30 25](https://user-images.githubusercontent.com/6597771/51270444-af203d00-19a3-11e9-9bb5-66c8cf5b9830.png)
CASE B: (simple conflict resolution dialog - edit local)
EDIT LOCAL
andLOAD REMOTE
CASE C: (simple conflict resolution dialog - load remote)
EDIT LOCAL
andLOAD REMOTE
Update release notes:
RELEASE-NOTES.txt
.