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

Login modal #1785

Merged
merged 67 commits into from
Jul 22, 2019
Merged

Login modal #1785

merged 67 commits into from
Jul 22, 2019

Conversation

RachitMalik12
Copy link
Collaborator

@RachitMalik12 RachitMalik12 commented Jul 12, 2019

This PR introduces a login form in FROG to allow the user to login with their email and password. Furthermore, it renders the correct information on the TopBar of Single Activity depending on whether the user is logged in or logged out. The ability to switch between logout and sign up has also been implemented. I have moved the call to the server out of Sign Up and Login and into AccountModal. This makes SignUp and Login dumb components and they can be tested using storybook in the future.

Testing instructions

  • Click login and try to login with an email and password combination that you created.
  • If you don't fill either email or password you should get an alert.
  • Appropriate errors will be displayed as an alert (in the future a toast notification)
  • Once you are logged in, your display name will be displayed beside a logout button.

@RachitMalik12 RachitMalik12 requested review from LukasHostettler, houshuang and qgolsteyn and removed request for LukasHostettler July 19, 2019 10:03
@RachitMalik12 RachitMalik12 marked this pull request as ready for review July 19, 2019 10:04
Copy link
Collaborator

@houshuang houshuang left a comment

Choose a reason for hiding this comment

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

  • seems like the page reloads after logout? Not a big deal but...
  • not sure we need a notification when login is successful, since modal goes away and top bar changes... also not a big deal (especially with Toasts, which are not modal)
  • do we need two buttons for Login / Create Account?
  • the way you show the name when logged in (with the first letter in a circle) is particular - why did you choose this? Are you planning to use this design pattern anywhere else?

@houshuang
Copy link
Collaborator

Hi Rachit, great work. I added a few comments above, none of these are deal-breakers though, but I'd like your thoughts on them. Otherwise this should be good to go.

I updated the Asana board with the next steps as I see them, in order of execution. Have a look and see if you agree. Some of these are more complex and will need some discussion. https://app.asana.com/0/1129712605359574/1129712605359577

@RachitMalik12
Copy link
Collaborator Author

  • seems like the page reloads after logout? Not a big deal but...

Earlier, I wasn't able to get single activity to render on pushing "/". I changed the routing and it should now not reload. Pushed that change..

  • not sure we need a notification when login is successful, since modal goes away and top bar changes... also not a big deal (especially with Toasts, which are not modal)

Yes I think I will remove the alert. Good point.

  • do we need two buttons for Login / Create Account?

That was the option I thought would be best. Just to clarify, did you mean have one button and display the correct modal accordingly. Also that works because we can switch from within the modal. I however, still feel its nice to have 2 buttons. Open to discussion :)

  • the way you show the name when logged in (with the first letter in a circle) is particular - why did you choose this? Are you planning to use this design pattern anywhere else?

The reason for this was because display name doesn't have a notion of a first and last name (usually avatar's have the first letter of the first name and first letter of the last name), so I decided to use just the first letter of the display name. Again, @qgolsteyn is working on a TopBar component for all of FROG, which will have an avatar. Open to further discussion on this..

autoComplete="email"
autoFocus
/>
<TextField
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the future we could also create our own TextField component, if almost all of the props are identical.

@houshuang houshuang merged commit 30fb5ae into develop Jul 22, 2019
@houshuang houshuang deleted the login_modal branch July 22, 2019 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants