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

Don't show blocking alert on set feature image #11895

Merged

Conversation

@jklausa
Copy link
Member

commented Jun 12, 2019

Fixes #11433.

Before, trying to set a Featured image on a post while offline/on a slow connection, would throw up this ugly modal pop-up.

Now, it lets you pick an image and tries to upload it anyway — displaying a notice if it fails.

before after
Simulator Screen Shot - iPhone Xs Max - 2019-06-12 at 05 40 33 Simulator Screen Shot - iPhone Xs Max - 2019-06-12 at 05 33 20

To test:

  1. Put your device/sim offline.
  2. Go to Posts
  3. Open editor for any given Post
  4. Go to Post Settings
  5. Try to set a featured image
  6. Verify that the app lets you try to do that, even when offline

@jklausa jklausa requested a review from yaelirub Jun 12, 2019

@jklausa jklausa self-assigned this Jun 12, 2019

@jklausa jklausa added this to the 12.7 milestone Jun 12, 2019

successBlock();
}
} failure:^(NSError * _Nonnull error) {
if ([error.domain isEqualToString:NSURLErrorDomain] && weakSelf.ignoreNetworkLoadingErrors && successBlock) {

This comment has been minimized.

Copy link
@yaelirub

yaelirub Jun 12, 2019

Contributor

Do we need to check the error domain here? or can we handle the same for any error?

This comment has been minimized.

Copy link
@jklausa

jklausa Jun 12, 2019

Author Member

I was being cautious here — I think it might be slightly dangerous to outright ignore any errors this method can return — if there's a failure when parsing the response or a Core Data error, I don't think we want to swallow them the same way.

I'm open to changing this and being more liberal if you think it's fine though :)

This comment has been minimized.

Copy link
@yaelirub

yaelirub Jun 12, 2019

Contributor

Will we still see the annoying popup alert error on any of the other cases (like core data error). I believe we made a general design decision to not block the user with these annoying alerts on errors anymore unless the errors should block progress.
What do you think?

This comment has been minimized.

Copy link
@jklausa

jklausa Jun 12, 2019

Author Member

Yeah, you're right. I changed it to handle any errors here.


WPAndDeviceMediaLibraryDataSource *mediaDataSource = [[WPAndDeviceMediaLibraryDataSource alloc] initWithPost:self.apost
initialDataSourceType:MediaPickerDataSourceTypeMediaLibrary];
mediaDataSource.mediaLibraryIgnoresNetworkLoadingErrors = YES;

This comment has been minimized.

Copy link
@yaelirub

yaelirub Jun 12, 2019

Contributor

Not sure I understand what we're doing here. If we are always setting the mediaLibraryIgnoresNetworkLoadingErrors to true why even use it?

This comment has been minimized.

Copy link
@jklausa

jklausa Jun 12, 2019

Author Member

WPAndDeviceMediaLibraryDataSource is also used in different places and I didn't want to silently change the behavior for other screens in the app

This comment has been minimized.

Copy link
@yaelirub

yaelirub Jun 12, 2019

Contributor

Oh! I see it's used in site creation. What happens there when picking media in offline?

This comment has been minimized.

Copy link
@jklausa

jklausa Jun 12, 2019

Author Member

the lower-level MediaLibraryPickerDataSource is also used in bunch of other places, like GutenbergMediaPickerHelper and AztecPostViewController, PostEditor, etc.

I don't want to change the behavior for those.

the WPAndDeviceMediaLibraryDataSource that wraps both local images and MediaPicker is also used in picking the site icon — it's actually a great point you raised and I changed it to work in both places correctly.

@yaelirub
Copy link
Contributor

left a comment

Not sure I understand the use of the new boolean mediaLibraryIgnoresNetworkLoadingErrors and why we need it if we are always setting it to true.

The copy for the error message seems a bit off to me. I'm not sure we need the message. Maybe we could just have the title of "Couldn't upload feature image" or something like "Error occurred while uploading featured image"?
Maybe design can give some input here.

@yaelirub

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

@jklausa , I think this PR should include an update to release notes as well.

@jklausa

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

The copy for the error message seems a bit off to me. I'm not sure we need the message. Maybe we could just have the title of "Couldn't upload feature image" or something like "Error occurred while uploading featured image"?

I'll add a "Needs Copy Review" label, but for what it's worth it's not a new message — I'm reusing what was in the app already.

@mikedang

This comment has been minimized.

Copy link

commented Jun 12, 2019

I'll add a "Needs Copy Review" label, but for what it's worth it's not a new message — I'm reusing what was in the app already.

The current copy is grammatically off. We could say something like:

There was a problem with accessing your media. Please try again.

or

An error occurred while uploading your media. Please try again.

@jklausa

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

@mikedang sorry for being unclear here.

I meant that, in the screenshot on the right, indicating "after", that is gonna be displayed after this change, I'm using a copy that was already in use elsewhere in the app — I didn't write it just for this. If you have suggestions as to how improve that — I'm all ears :)

@jklausa jklausa requested a review from yaelirub Jun 12, 2019

@jklausa

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

@yaelirub should be ready for another go!

@yaelirub
Copy link
Contributor

left a comment

@jklausa , thank you for your responses. LGTM

@jklausa

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

awesome! if there are copy changes suggested by @mikedang, I'll file them as a separate PR :)

@jklausa jklausa merged commit 2bfc9f5 into develop Jun 12, 2019

3 checks passed

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

@jklausa jklausa deleted the feature/11433-dont-show-blocking-alert-on-set-feature-image branch Jun 12, 2019

@mikedang

This comment has been minimized.

Copy link

commented Jun 12, 2019

Ah, I see. Thanks for clarifying, @jklausa. I think the current message works fine.

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