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.

Sign up for GitHub

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

Jump to bottom

Added UnrecoverableError support and retry links #419

Merged
merged 4 commits into from
Apr 27, 2017
Merged

Added UnrecoverableError support and retry links #419

merged 4 commits into from
Apr 27, 2017

Conversation

cocojoe
Copy link
Member

@cocojoe cocojoe commented Mar 27, 2017

Unrecoverable asset added
Added i18n Strings
Update Tests
Update SwiftLint

@cocojoe
Copy link
Member Author

cocojoe commented Mar 27, 2017

simulator screen shot 27 mar 2017 14 58 45
simulator screen shot 27 mar 2017 15 03 11

hzalaz
hzalaz suggested changes Mar 28, 2017
View reviewed changes
Lock/UnrecoverableErrorPresenter.swift Outdated
}
} else {
view.secondaryButton?.onPress = { _ in
UIApplication.shared.openURL(URL(string: "https://auth0.com/docs/")!)
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you are asking about implementation, we already have a button in place so better to use vs removing the button and messing around with NSLinkAttributeName.

If you are asking why does it link to auth0/docs, this is the behaviour in Android.
auth0/Lock.Android#407

Lock/UnrecoverableErrorView.swift Outdated
actionButton.button?.setTitleColor(UIColor(red:0.04, green:0.53, blue:0.69, alpha:1.0), for: .normal)
actionButton.button?.titleLabel?.font = actionLabel.font

if !canRetry {
Copy link
Member

Choose a reason for hiding this comment

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

Do the positive first then the rest since its cleaner

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Lock/UnrecoverableErrorView.swift Outdated

if !canRetry {
actionLabel.text = "Please contact ".i18n(key: "com.auth0.lock.error.unrecoverable.support.title", comment: "Support label")
actionButton.title = "support.".i18n(key: "com.auth0.lock.error.unrecoverable.support.action", comment: "Support action")
Copy link
Member

Choose a reason for hiding this comment

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

Why splitting the error message instead of using formatting and a single button?

Copy link
Member

Choose a reason for hiding this comment

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

Also the main title needs a little more love since Please retry seems a bit odd

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be far simpler to use a secondaryButton, I was adhering to the original design which is consistent with Android.

I agree the title is a little sparse, I also feel in general this has been designed like a mobile web page vs a native app.
I think it would be better to have the call to action using a PrimaryButton.

However in the past when I've played with the design you then say it doesn't match the original design...

@hzalaz hzalaz added this to the v2-Next milestone Mar 28, 2017
@cocojoe
Copy link
Member Author

cocojoe commented Mar 28, 2017

Original

screen shot 2017-03-28 at 10 17 10

@cocojoe cocojoe force-pushed the added-error-screens branch 2 times, most recently from abd8f48 to 3432197 Compare April 3, 2017 10:20
@cocojoe
Copy link
Member Author

cocojoe commented Apr 3, 2017

simulator screen shot 3 apr 2017 11 02 40
simulator screen shot 3 apr 2017 11 01 39
simulator screen shot 3 apr 2017 11 00 54

hzalaz
hzalaz suggested changes Apr 4, 2017
View reviewed changes
Lock/OptionBuildable.swift Outdated
@@ -36,6 +36,9 @@ public protocol OptionBuildable: Options {
/// Privacy Policy URL. By default is Auth0's.
var privacyPolicyURL: URL { get set }

/// Support URL. By default this is not set.
Copy link
Member

Choose a reason for hiding this comment

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

Support page url that will be displayed (Inside Safari) when an unrecoverable error occurs and the user taps the "Contact Support" button in the error screen

Copy link
Member

Choose a reason for hiding this comment

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

By default no page should be displayed, not even auth0's so the button should be hidden

Copy link
Member Author

Choose a reason for hiding this comment

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

It is hidden by default, only visible when supportURL is populated as per last screenshot. Will update the docheader though.

Lock/OptionBuildable.swift Outdated
@@ -136,4 +139,16 @@ public extension OptionBuildable {
}
}

/// SupportURL. By default is not set.
var support: String {
Copy link
Member

Choose a reason for hiding this comment

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

Let it be nullable and call it supportPage since it might be odd to have just support

@cocojoe cocojoe force-pushed the added-error-screens branch 2 times, most recently from 7b8b70c to 2e977ee Compare April 7, 2017 12:38
@cocojoe cocojoe force-pushed the added-error-screens branch 2 times, most recently from 6dbfc06 to 98f78f4 Compare April 20, 2017 10:18
@cocojoe
Copy link
Member Author

cocojoe commented Apr 24, 2017

simulator screen shot 24 apr 2017 16 56 16
simulator screen shot 24 apr 2017 16 57 08
simulator screen shot 24 apr 2017 16 58 50

@lbalmaceda lbalmaceda self-requested a review April 26, 2017 13:59
lbalmaceda
lbalmaceda previously requested changes Apr 26, 2017
View reviewed changes
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

Texts are ok and equal to the other's sdks. I left you a few comments.

Lock/Base.lproj/Lock.strings Outdated
// Recoverable error message
"com.auth0.lock.error.recoverable.message" = "Please check your internet connection.";
// Recoverable error button
"com.auth0.lock.error.recoverable.retry.button" = "Retry";
Copy link
Contributor

Choose a reason for hiding this comment

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

The key should be named com.auth0.lock.error.recoverable.button (without the retry), to be similar to the "contact support" one: com.auth0.lock.error.unrecoverable.button

Lock/OptionBuildable.swift
return url.absoluteString
}
set {
guard let value = newValue, let url = URL(string: value) else { return } // FIXME: log error
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's time to remove those notes and log the error? 😝 It can be in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

One day :)

@cocojoe
Refactored UnrecoverableError View
a65185f 
Unrecoverable assets added
Added i18n Strings
Update Tests
@cocojoe
Updated Design
c55b1b5 
Simplified UnrecoverableErrorView
Added optional 'supportURL' to be displayed on non recoverable errors
Added Options Tests
Updated UnrecoverableError Tests
@cocojoe
Update Unrecoverable Design & Copy with latest spec
20b0045
@cocojoe
Update i18n reference
914db26
@hzalaz hzalaz dismissed lbalmaceda’s stale review April 27, 2017 13:44

Already completed

@hzalaz hzalaz merged commit 2b453b7 into master Apr 27, 2017
@hzalaz hzalaz deleted the added-error-screens branch April 27, 2017 13:44
@cocojoe cocojoe mentioned this pull request Jun 6, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzalaz hzalaz hzalaz approved these changes

@lbalmaceda lbalmaceda lbalmaceda left review comments

Assignees
No one assigned
Labels
CH: Added
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@cocojoe @hzalaz @lbalmaceda