Skip to content

Conversation

@diegoreymendez
Copy link
Contributor

@diegoreymendez diegoreymendez commented Jul 22, 2019

Initial step towards implementing #12227

This PR implements the following changes:

  • Changes the messages seen in the notices when you save posts while offline (a different message for saved / published / scheduled posts).
  • Changes the "Retry" button in the post list to a "Cancel" button, and the logic for it.
  • Changes the message and the color of the message shown in the post list for posts with unsaved changes and for posts that were saved while offline.
  • Fixes a bug in the post list that would hide the Retry / Cancel button. This bug was introduced by some previous changes in develop but was particularly annoying and evident while testing this PR.
  • Modifies notice views so that they can show 2 line titles. (fyi @frosty)
  • Modifies post cards so that they can show 2 line messages.

Demo:

offlinePublishing

Scope:

This PR doesn't aim to implement all of the changes we need for #11425. This PR aims to implement the basic messaging changes, some UI fixes, and replacing the "Retry" logic with a "Cancel" logic.

Testing:

Test 1: Notices

Try editing or creating a post while offline, and save the changes.
The notice shown should read:

"Post will be saved / scheduled / published the next time your device is online"

Test 2: Cancelling

  1. Try editing or creating a post while offline, and save the changes.
  2. Try pressing "Cancel" in the notice that comes up.
  3. Alternatively try pressing "Cancel" in the post list.

Make sure the post upload is cancelled and that the post is listed as having local changes.

Test 3: Post list text color

  1. Try editing or creating a post while offline, and save the changes.
  2. Make sure the text color is yellow (warning) in the post list.
  3. Make sure the button color for "Cancel" is yellow (warning) in the post list.
  4. Tap "Cancel" in the post list.

Make sure the text is now red (error). Also make sure the text reads "Post has local unsaved changes"

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@diegoreymendez
Copy link
Contributor Author

@osullivanchris - I added you as a reviewer for this issue, in case you want to comment from the design point of view.

@diegoreymendez diegoreymendez self-assigned this Jul 22, 2019
@diegoreymendez diegoreymendez added this to the 13.0 milestone Jul 22, 2019
@diegoreymendez diegoreymendez changed the title Issue/11425 improve error messages for offline saving Improve messages for offline saving Jul 22, 2019
@osullivanchris
Copy link

Small visual thing. For the cancel action, the icon should be flipped to face anti-clockwise. Also the text should match the icon colour.

Otherwise looks good on first impression. Is there a way I can test it?

@shiki
Copy link
Contributor

shiki commented Jul 23, 2019

I'm trying to understand what's going on here. Please bear with me.

I understand that this does not try to implement everything in #11425. However, I would like to clarify this new behavior that this PR introduces. I'm not sure if we would like to merge this, possibly making it to production, as it is right now.

Here is the flow that I see:

untitled@2x

Notice that:

  • The (b) screen shows that when publishing while offline, the post does not stay in Drafts list. It is in Published list (c).
  • After clicking on cancel, the post stays in Published list (d).

These points contradict the desired design described in #11425 where:

  • When publishing offline, the post should stay in Drafts list.
  • After cancelling, the post should be in Drafts.

Am I missing something? Is this how the flow should be?

@diegoreymendez diegoreymendez changed the title Improve messages for offline saving Full Offline Publishing Experience Jul 23, 2019
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jul 23, 2019

Warnings
⚠️ This PR is tagged with 'DO NOT MERGE'.
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@diegoreymendez
Copy link
Contributor Author

@shiki - There's no rush in merging this PR, I'll add the "Do not merge label" for the time being, but I'd like the discussion to continue.

A few notes on things we should probably change in this PR:

  1. Maybe we can add safeguards to avoid publishing old stuff on the first run of this new logic?
  2. Do we need to feature flag this?
  3. I think we're currently missing a max retry count for uploads. I'll make sure to add it.
  4. I'd consider merging Post published while offline will now be published once back online. #12149 into this PR since it's pretty much related.

Now regarding your points, I'll go back to the drawing board to see how I can make that happen. This is something @osullivanchris mentioned to me as well. The logic for knowing whether a post is being published or not isn't 100% easy, so it may take some playing around with status and remoteStatus.

@diegoreymendez
Copy link
Contributor Author

I'm marking this as not ready for review to go back to making some changes.

@jkmassel
Copy link
Contributor

jkmassel commented Jul 30, 2019

I'm bumping this to 13.1 as part of the code freeze. If we'd like to release it as part of 13.0, just merge it into release/13.0, and DM me – I'll be happy to cut a new beta build!

@jkmassel jkmassel modified the milestones: 13.0, 13.1 Jul 30, 2019
@shiki shiki self-assigned this Jul 30, 2019
@shiki
Copy link
Contributor

shiki commented Jul 30, 2019

I'm currently trying to take this apart (split it into multiple PRs). The plan is described in #12240.

@diegoreymendez
Copy link
Contributor Author

diegoreymendez commented Jul 31, 2019

@shiki - Are you planning to split the existing code into smaller PRs or start from scratch?

The main reason I was holding back on this PR is because I want to make sure that first we have:

  1. A timer that will auto-cancel old upload requests.
  2. The user-confirmation safety checks (apparently a hash code, unless we decide on something different).

These two safety measures are top priority, so maybe we can chat about them tomorrow.

I think it's extremely important that we make sure those are in place before moving forward with auto-uploads again.

@shiki
Copy link
Contributor

shiki commented Jul 31, 2019

Are you planning to split the existing code into smaller PRs or start from scratch?

@diegoreymendez I'm gonna reuse your commits. I'm targetting a master branch to make sure we have everything we need (like you said) before we merge anything. With the master branch, there's some leeway on the sequence and more people can contribute.

@diegoreymendez
Copy link
Contributor Author

I'm closing this since it was split into newer / smaller PRs and is no longer relevant.

@diegoreymendez diegoreymendez deleted the issue/11425--improve-error-messages-for-offline-saving branch September 14, 2019 13:39
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.

5 participants