Login: Social Error Screens by nheagy · Pull Request #7887 · wordpress-mobile/WordPress-iOS

nheagy · 2017-10-02T05:14:37Z

Error display screen for when login to Google succeeds, but login using the token with WordPress.com subsequently fails.

To test:

login to a Google account with an email that doesn't have a wpcom account
ensure error screen matches mockup

Note:

This is forked from #7872 and can't be merged until that PR is approved & merged.

Needs review: @aerych

houndci-bot · 2017-10-02T05:18:38Z

+
+// MARK: UITableViewDataSource methods
+
+extension LoginSocialErrorViewController


Opening Brace Spacing Violation: Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)

aerych · 2017-10-02T22:00:22Z

Heya @nheagy!
Two quick visual things I noticed and can share before getting too deep into the code review.

The nav bar is missing the W logo and the Help button.
The font color of the cell text is black. It should one of the dark grey shades judging from the comp. @folletto can say for sure but maybe darkGrey() to match text use elsewhere? https://cloudup.com/ilmApZlpV4i

aerych

Hiya Nate! Here are a few thoughts after peeking at the code and playing with the screen in the simulator. I had a couple of suggestions. Overall the code is very pretty. :) Nice yo!

aerych · 2017-10-02T22:05:06Z

-        configureViewLoading(false)
+        if awaitingGoogle {
+            let socialErrorVC = LoginSocialErrorViewController(title: NSLocalizedString("Unable To Connect", comment: "Shown when a user logs in with Google but it subsequently fails to work as login to WordPress.com"), description: error.localizedDescription)
+            navigationController?.setViewControllers([socialErrorVC], animated: true)


Is this a good place to set awaitingGoogle back to false?

Yes, tho I expect this VC will get dealloced because the navigation controller is about to release it.

aerych · 2017-10-02T22:06:18Z

+            let socialErrorVC = LoginSocialErrorViewController(title: NSLocalizedString("Unable To Connect", comment: "Shown when a user logs in with Google but it subsequently fails to work as login to WordPress.com"), description: error.localizedDescription)
+            navigationController?.setViewControllers([socialErrorVC], animated: true)
+        } else {
+            configureViewLoading(false)


Shouldn't we still set configureViewLoading(false) regardless of the value of awaitingGoogle?

Yeah, probably. Same thinking as above—currently this method destroys the current VC so ¯\(ツ)/¯

But I can see that a future refactoring may miss the need to make these changes, so perhaps they make sense regardless 😀

aerych · 2017-10-02T22:10:26Z

+        descriptionLabel.font = WPStyleGuide.mediumWeightFont(forStyle: .subheadline)
+        descriptionLabel.textColor = WPStyleGuide.darkGrey()
+        descriptionLabel.numberOfLines = 0
+        descriptionLabel.heightAnchor.constraint(greaterThanOrEqualToConstant: 14.0).isActive = true


Should this constant be a ... constant?

aerych · 2017-10-02T22:13:56Z

+import Foundation
+import Gridicons
+
+class LoginSocialErrorViewController: UITableViewController {


This might have been a good candidate for ImmuTable. Just FYI. No need to refactor.

Ah, right. Hadn't even crossed my mind.

aerych · 2017-10-02T22:21:20Z

+
+        let storyboard = UIStoryboard(name: "Login", bundle: nil)
+        if let controller = storyboard.instantiateViewController(withIdentifier: controllerKey) as? NUXAbstractViewController {
+            navigationController?.setViewControllers([controller], animated: true)


I'm not sure about this. Summoning @folletto.
When I first tapped the email option to retry another view controller appeared to be pushed onto the stack. My expectation, since the error screen was pushed onto the nav stack, was that I'd go back a screen. But then there's no back button so that's a bit odd.
When I tried the option to sign up I was trapped with no way to get back to the login flow.
This is similar for the site address screen... there is no way to return to the email screen.

Given the scenarios above I think it would make sense to present the error screen as a modal and maybe let the presenting controller decide what action to take via a delegate method. Thoughts?

Ya, I could see model presentation + delegate as a nice UX.

I did it this way because of the navigation bar being part of these screens. I'd also like to hear @folletto's take 😀

I guess I need to ask a screenshot of the 2+ screens you're discussing about. It's a bit too theoretical in writing :D

@folletto here's a demonstration: http://d.pr/v/bX1dEZ

Producing that video made it clear that @aerych has a point. A bit of direction on how best to alter it please 😀

The "← Back" action should be present there, behaving exactly like the user just opened that screen in a normal flow of actions, which means that the "Back":

On the site address screen, it should go back to the email screen (where normally the user would select to enter site address).

On the email screen, it should go back to the intro screen.

This is because these actions there act as "shortcuts" into a specific part of the flow.

The "← Back" action should be present there, behaving exactly like the user just opened that screen in a normal flow of actions

I have concerns about this. In the video example Nate shared new screens are sliding in from the right and the error screen is transitioned away by sliding to the left... a typical navigation experience. This implies that tapping back will take the user to the error screen, but if I understand correctly, we're suggesting that we break this expectation. So the experience would be the error screen slides in, the user taps an option on the error screen, a new screen slides in. The user taps the back button... but rather than seeing the error screen they see something different. I expect this would be confusing.

I think it would be better for the error screen presented modally so it slides up from the bottom of the screen and dismisses by sliding down. This removes any expectation that it is something the user would see by tapping a back button.

Yep — the sliding motion should be different too with this model.

If we want to do it modal we need to go back to the design and rework both iOS and Android at this point.

Caught up with @folletto in chat. To summarize ... we're talking about the same thing but speaking in different terms. :D So let's go ahead and present the error screen "modally" (such an ambiguous term), full screen, sliding up from the bottom, etc. so we avoid any weirdness with user expectations and the back button. :)

aerych · 2017-10-02T22:22:11Z

+// MARK: UITableViewDelegate methods
+
+extension LoginSocialErrorViewController {
+    override func tableView(_ tableView: UITableView, estimatedHeightForRowAt indexPath: IndexPath) -> CGFloat {


I think this method isn't needed when setting tableView.estimatedRowHeight

Ah damn, right.

aerych · 2017-10-02T22:23:25Z

+
+        tableView.estimatedRowHeight = 100.0
+
+        view.backgroundColor = WPStyleGuide.greyLighten30()


Seems like these properties would be better set on viewDidLoad since its work that should only need to be done once.

aerych · 2017-10-02T22:29:46Z

+    }
+
+    override func tableView(_ tableView: UITableView, heightForFooterInSection section: Int) -> CGFloat {
+        return 0.5


I think this method can be removed if tableView:viewForFooterInSection: returns UIView(frame: .zero). Seems that way when I test. Same for setting the footer.backgroundColor

This draws the dark line above and below the button cells.

The initial frame size of .zero will be ignored/modified by autolayout.

I totally missed that there were lines in the mock. Totally makes sense now. :)

The initial frame size of .zero will be ignored/modified by autolayout

This hasn't been my experience, but whatev 🤷‍♂️

I usually just do UIView() not sure why I decided to use UIView(frame: .zero) here 😜 I'll probably switch it to the former, to make the result more clear.

aerych · 2017-10-02T22:56:33Z

Something I've been thinking about is how to best share some of our existing screens, like 2fa and the wpcom password screen, while performing a google login flow.
One idea is to add a new flag to LoginFields but its starting to be a bit of a dumping ground for state and its not good to keep piling on. Its something we need to sort for the 2fa and password flows and probably would include the use of awaitingGoogle in this PR. Not something that needs to be solved for this PR, but if you have ideas I'm all ears.

nheagy · 2017-10-03T04:23:39Z

I agree that we need a way to maintain state between all the login screens. And I agree that LoginFields isn't designed to hold a variety of data elegantly, but I think it could possibly be refactored to do so? I'd like to give that a short first, at least.

nheagy · 2017-10-04T20:18:33Z

Good call on the button text color. The mockup is darker than the handbook's Gray Dark/darkGrey (#2e4453)—I sampled it as #163B4C, which is still definitely not black 😉 I switched it to darkGrey, hoping @folletto can say if that's ok.

aerych · 2017-10-05T01:07:08Z

Good call on the button text color

Did you mean the cell text color? Not sure we're talking about the same thing? Fwiw, Digital Color Meter shows it as #000000, even in the screen shot above.:

nheagy · 2017-10-05T01:46:45Z

@aerych I know the PR has the text as black. I’m referring to the color used in the mockups - it is darker than our darkGrey.

aerych · 2017-10-05T02:04:50Z

Ahh. Gotcha! 👍

folletto · 2017-10-05T11:28:08Z

Ah thanks for that review of text color. As a general rule of thumb:

We don't use black most of the time — if it's black it's likely an oversight and should be darkGrey
If the color looks outside the color palette, it's likely an artifact, as with some exceptions we tend to stick to the palette.

nheagy · 2017-10-06T14:14:47Z

Ok, @aerych this should be good for review #2!

aerych

Heya @nheagy. Changes look great and the interaction with the modal feels soooo much better. after resolving the conflicts :)

aerych · 2017-10-06T20:42:51Z

+        let imageView = UIImageView(image: image.imageWithTintColor(UIColor.white))
+        navigationItem.titleView = imageView
+    }
+    func addHelpButtonToNavController() -> (UIButton, WPNUXHelpBadgeLabel) {


nice tupile usage :)

aerych · 2017-10-06T20:43:10Z

 import Gridicons

-class LoginViewController: NUXAbstractViewController {
+protocol LoginWithLogoAndHelpViewController {


Nice refactor :)

nheagy added the Login label Oct 2, 2017

nheagy added this to the 8.6 milestone Oct 2, 2017

nheagy self-assigned this Oct 2, 2017

nheagy requested a review from aerych October 2, 2017 05:14

nheagy added the [Status] DO NOT MERGE label Oct 2, 2017

nheagy mentioned this pull request Oct 2, 2017

Feature/7675 social login service call #7872

Merged

houndci-bot reviewed Oct 2, 2017

View reviewed changes

nheagy added 6 commits October 2, 2017 15:04

Add error screen VC

8e372dd

Add social login error action to try again buttons

be1e614

Add signup button to social login error

e38ed89

Add display of error screen to google login

64a020b

fix a brace from a really bad bad brace to an obviously better one

bc51ab8

Add missing new entries to project file

0ec7017

nheagy force-pushed the feature/7675-error-screens branch from 5d15084 to 0ec7017 Compare October 2, 2017 21:05

aerych suggested changes Oct 2, 2017

View reviewed changes

nheagy mentioned this pull request Oct 3, 2017

Login: fix Google button covering error label #7902

Merged

nheagy added 5 commits October 4, 2017 14:23

Change button color to darkGrey

eb709dc

Set awaitingGoogle back to false, cleanup configureViewLoading

1d6228a

Make min height value a Constant

2af050a

Move bg color to viewDidLoad; clean up estimated height code

42d648c

Remove frame: .zero init calls

c93e916

Add some comments to LoginSocialErrorViewController

e30271b

nheagy added 2 commits October 5, 2017 14:16

Switch social error VC to use modal presentation

fee9eda

Add help button and WP logo to social error screen

677a8d0

aerych approved these changes Oct 6, 2017

View reviewed changes

elibud modified the milestones: 8.6, 8.7 Oct 9, 2017

Merge branch 'develop' into feature/7675-error-screens

d63f41d

nheagy merged commit f551b01 into develop Oct 11, 2017

nheagy deleted the feature/7675-error-screens branch October 11, 2017 18:00

nheagy removed the [Status] DO NOT MERGE label Oct 11, 2017

nheagy mentioned this pull request Oct 13, 2017

Login: Google Master Thread #7675

Closed

17 tasks


		// MARK: UITableViewDataSource methods

		extension LoginSocialErrorViewController


		tableView.estimatedRowHeight = 100.0

		view.backgroundColor = WPStyleGuide.greyLighten30()

Uh oh!

Conversation

nheagy commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aerych commented Oct 2, 2017

Uh oh!

aerych left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nheagy Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nheagy Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aerych commented Oct 2, 2017

Uh oh!

nheagy commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nheagy commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aerych commented Oct 5, 2017

Uh oh!

nheagy commented Oct 5, 2017

Uh oh!

aerych commented Oct 5, 2017

Uh oh!

folletto commented Oct 5, 2017

Uh oh!

nheagy commented Oct 6, 2017

Uh oh!

aerych left a comment

nheagy commented Oct 2, 2017 •

edited

Loading

nheagy Oct 3, 2017 •

edited

Loading

nheagy Oct 3, 2017 •

edited

Loading

nheagy commented Oct 3, 2017 •

edited

Loading

nheagy commented Oct 4, 2017 •

edited

Loading