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

remove blocking spinner on upload and autosave before preview #12031

Merged
merged 9 commits into from Jul 11, 2019

Conversation

@yaelirub
Copy link
Contributor

commented Jun 29, 2019

Fixes #11812

As described in the issue when saving a draft or autosaving a post before preview we used to show a blocking spinner which would block the user from performing any actions till preview is generated.
This issue is prominent in low connectivity.

This PR aims to improve the experience by showing the saving/generating status in the navigation bar the same way we do when uploading media.

generating

This improvement brings up the need to also remove the blocking spinner when loading the preview (web view loading). It will be tackled in a separate PR

To test:

  1. Go to device settings -> Developer and turn on Network Link Conditioner with "Very Bad Network"
    POSTS:
  2. Open the app and go to published posts
  3. Update a post
  4. tap preview
  5. See "Generating Preview" in the nav bar.
  6. Make sure you can exist the editor and perform other actions.
  7. Go back from the preview and see that the navigation bar in the editor is back to default.

DRAFTS:
2. Open the app and go to drafts
3. Update a draft
4. tap preview
5. See "Saving Draft" in the nav bar.
6. Make sure you can exist the editor and perform other actions.
7. Go back from the preview and see that the navigation bar in the editor is back to default.

Update release notes:

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

@yaelirub yaelirub added this to the 12.9 milestone Jun 29, 2019

@yaelirub yaelirub added this to In Review in Offline Support: Posting [iOS] via automation Jun 29, 2019

@yaelirub yaelirub requested review from diegoreymendez and jklausa Jun 29, 2019

@jklausa

jklausa approved these changes Jul 2, 2019

Copy link
Member

left a comment

I checked it out in simulator with Link Conditioner enabled on my Mac and it works exactly as described!

The code looks good, but in testing this I have two notes:

  1. It's kinda hard to notice the fact something has changed in the navbar - I'm not sure if I didn't know where to look, I'd notice anything at all. Users will probably get used to it though.

A bigger problem however, is that the amount of text we can fit in that amount of space is extremely limited - I'm worried about localisation issues here. With double-length pseudolanguage enabled, it already doesn't fit (and causes UI glitch with the spinner):
Simulator Screen Shot - iPhone Xs Max - 2019-07-02 at 02 54 31

Another issue is that because the preview loading still hapens synchronously it's possible to get into this weird state where you try to leave the screen while the new async preview generation happens, but you don't do it fast enough and get blocked in your interaction - I imagine this will get solved in the future PR you mentioned for async preview loading - but I thought I'd mention it just in case - maybe something to keep in mind for the next PR :)
Simulator Screen Shot - iPhone Xs Max - 2019-07-02 at 02 49 47

@yaelirub

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

It's kinda hard to notice the fact something has changed in the navbar - I'm not sure if I didn't know where to look, I'd notice anything at all. Users will probably get used to it though.

A bigger problem however, is that the amount of text we can fit in that amount of space is extremely limited - I'm worried about localization issues here. With double-length pseudolanguage enabled, it already doesn't fit:

Hey @jklausa , thank you so much for these comments. @osullivanchris do you have any input on these?

(and causes UI glitch with the spinner)

I will update the code to fix this. Wish we were using autolayout there.

I imagine this will get solved in the future PR you mentioned for async preview loading - but I thought I'd mention it just in case - maybe something to keep in mind for the next PR :)

It will be solved in a future PR. Thanks for mentioning though. 🙏

@osullivanchris

This comment has been minimized.

Copy link

commented Jul 2, 2019

Hey @jklausa , thank you so much for these comments. @osullivanchris do you have any input on these?

Hey I think these concerns from @jklausa are totally valid! I thought it was effective to grab something for free here to unblock us. But if I was designing this pattern from scratch that's not the way I would do it. I actually don't know if I've seen that in any app before :D

It depends whether we think there is value in creating a new pattern for non-blocking loading indication. I'd be happy to take a look at it, but perhaps it should be a separate issue?

@jklausa

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Yeah — I think it's fine to ship this as-is, since it's still an improvement over current state — and we can revisisit and revamp it down the line if we have time :)

@yaelirub yaelirub force-pushed the issue/11812_generating_preview_progress_bar branch from a74ed65 to b4be965 Jul 3, 2019

@yaelirub yaelirub force-pushed the issue/11812_generating_preview_progress_bar branch from b4be965 to 9b1a5fe Jul 3, 2019

@yaelirub yaelirub force-pushed the issue/11812_generating_preview_progress_bar branch from 9b1a5fe to a2abd95 Jul 3, 2019

@yaelirub

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@jklausa , I have created a new view LoadingStatusView that is meant to replace the WPUplaodStatusButton.
It is using autolayout instead of frames so layout should be less problematic.

I tested this in English and Hebrew.

Please test like you did before and let me know what you think.

If it seems good to you I can open another PR to replace WPUplaodStatusButton with LoadingStatusView

@shiki

shiki approved these changes Jul 10, 2019

Copy link
Member

left a comment

Previewing may cause drafts to be published 🎈

This is something that we should fix sooner than later. To reproduce:

  1. Create a local draft.
  2. In the Editor, click on the options → Post Settings
  3. In Post Settings, set the Status to Published and then exit the Post Settings.
  4. Click on Preview

Since we now upload the post before previewing, the draft gets uploaded with a "Published" status. This is something that we recently learned the hard way. 😞

///
private lazy var previewGeneratingView: LoadingStatusView = {
let view = LoadingStatusView(title: NSLocalizedString("Generating Preview", comment: "Message to indicate progress of generating preview"))
view.translatesAutoresizingMaskIntoConstraints = false

This comment has been minimized.

Copy link
@shiki

shiki Jul 10, 2019

Member

⚠️ This looks unnecessary since LoadingStatusView already does this in its initializer.

if error != nil {
let title = NSLocalizedString("Preview Unavailable", comment: "Title on display preview error" )
self?.displayPreviewNotAvailable(title: title)
guard let self = self else {

This comment has been minimized.

Copy link
@shiki

shiki Jul 10, 2019

Member

Nice improvement!

@@ -3190,6 +3190,7 @@ extension AztecPostViewController {
static let defaultMargin = CGFloat(20)
static let blogPickerCompactSize = CGSize(width: 125, height: 30)
static let blogPickerRegularSize = CGSize(width: 300, height: 30)
static let savingDraftButtonSize = CGSize(width: 130, height: 30)

This comment has been minimized.

Copy link
@shiki

shiki Jul 10, 2019

Member

Looks like this is misaligned. 🙂

@yaelirub

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Following PR to replace WPUplaodStatusButton with LoadingStatusView

@yaelirub yaelirub merged commit d3bf6fb into develop Jul 11, 2019

3 checks passed

Hound No violations found. Woof!
Peril All green. Congrats.
Details
ci/circleci: build_and_test Your tests passed on CircleCI!
Details

Offline Support: Posting [iOS] automation moved this from In Review to Done (PRs) Jul 11, 2019

@yaelirub yaelirub deleted the issue/11812_generating_preview_progress_bar branch Jul 11, 2019

@designsimply

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

I tested with 12.9 beta and can confirm that when I edit a post then tap Preview I can see a saving/generating status in the navigation bar.

preview generating-preview
Tested with WP Internal 12.9.0.20190717 on iPhone 6S iOS 12.3.1 on a WordPress 5.2.2 Jetpack 7.5.3 site.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.