Skip to content

Add login with Google button - #7731

Merged
nheagy merged 9 commits into
developfrom
issues/7675-add-google-button
Sep 5, 2017
Merged

Add login with Google button#7731
nheagy merged 9 commits into
developfrom
issues/7675-add-google-button

Conversation

@nheagy

@nheagy nheagy commented Aug 25, 2017

Copy link
Copy Markdown
Contributor

Refs #7675

Add the Google login button that will launch the new login process (which currently doesn't exist).

simulator screen shot aug 25 2017 1 05 22 pm

To test:

  • Start the login process
  • Make sure you see the Google button
  • If you're a real keener, make sure it works in a variety of situations:

google login button scenarios

Needs review: @aerych

@nheagy nheagy added the Login label Aug 25, 2017
@nheagy nheagy self-assigned this Aug 25, 2017
@nheagy
nheagy requested a review from aerych August 25, 2017 19:38
])
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Trailing Newline Violation: Files should have a single trailing newline. (trailing_newline)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So you're the one they call the dog?

@elibud elibud added this to the 8.5 milestone Aug 28, 2017

@aerych aerych left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Heya Nate! This is really nice :)

Visually, I think we might need to use greyDarken30 vs darkGrey for the text in order to match the info label.

Otherwise, the only other suggestion I have would be to consider maybe subclassing UIButton vs an extension. I realize the articles advising against subclassing, but the more I think about it I think its only a bad practice if the subclassing means to override methods responsible for a UIButton's layout or behavior. The code here doesn't alter a UIButtons behavior. it just acts as a factory. I think extensions on framework classes make the most sense when the added functionality is rather general in purpose and would be used in multiple places. This is a specific uses case. An extension works, but a subclass seems a better fit to me. Not a blocker, final call is up to you.

Edit: Ya know.. given that the extension is just acting as a factory, maybe WPStyleGuide would also be a good fit. Just a thought.

@aerych

aerych commented Aug 28, 2017

Copy link
Copy Markdown
Contributor

simulator screen shot aug 28 2017 10 53 20 am
Really close, but the text in the google button is a bit darker

@nheagy

nheagy commented Aug 28, 2017

Copy link
Copy Markdown
Contributor Author

@aerych good catch on the text color. Totally looked the same at the time 😉

I feel like keeping this as an extension is fine for now, because it's working. At some point I want to fix the text buttons at the bottom of the login screens, and those will need to become some kind of class in order to be used in Storyboards.

@nheagy
nheagy merged commit 5461455 into develop Sep 5, 2017
@nheagy
nheagy deleted the issues/7675-add-google-button branch September 5, 2017 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants