Skip to content
This repository was archived by the owner on Sep 17, 2022. It is now read-only.

Conversation

@bjelke
Copy link
Contributor

@bjelke bjelke commented Oct 14, 2018

New layout for login page

@bjelke bjelke requested a review from gbrown3 October 14, 2018 19:13
Copy link
Contributor

@gbrown3 gbrown3 left a comment

Choose a reason for hiding this comment

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

Great work here, the login page looks a million times better. There are a few more issues we need to address before we can merge this, most of them functional.

  1. The Google signin button is a little messed up and I think it mostly has to do with the way I initially implemented it. The two main issues are that a prompt to sign in appears as soon as I open the page (without tapping anything) and then tapping the google sign in button doesn't do anything. I can't remember all the ins and outs of how to make it work, but this tutorial should serve you well.

  2. We need to handle errors, both from firebase login and google sign in. For example, if the user puts in an email that isn't in a valid format, there should be a popup message that tells them that. I think most if not all of these warnings can just be a simple popup message (I think they're called Alerts or UIAlerts in iOS).

  3. Just as good practice, I think we should have some tests for the isEmail() method. I put more detail about this in a comment below.

All in all, keep it up! (And don't worry about working on the caching thing, this seems like plenty to work on already)

// verify that input is a valid email format (not specifically macalester)
// the text box expects email input, but as far as I can tell it only uses that to set the keyboard
// code from: https://stackoverflow.com/a/35789191
static func isEmail(email:String) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good, but would be even better if we had some tests for it too. In the "The Mac Weekly Tests" folder you can find a swift file that has a few tests in it already. If you could add a few basic tests that would help me feel a little better about this regex working, especially seeing as I have a hard enough time figuring it out when I write it myself lol.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants