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

Allow Email login #412

Merged
merged 10 commits into from
Mar 11, 2024
Merged

Allow Email login #412

merged 10 commits into from
Mar 11, 2024

Conversation

kellyel
Copy link
Collaborator

@kellyel kellyel commented Mar 8, 2024

This PR adds the ability to log in using a password for email users.

Copy link

github-actions bot commented Mar 8, 2024

Visit the preview URL for this PR (updated for commit 92bbe5e):

https://roar-staging--pr412-enh-email-login-nmgpg136.web.app

(expires Fri, 15 Mar 2024 23:10:57 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 2631e9c58fd0104ecbfddd72a62245ddac467460

@kellyel kellyel requested review from richford, ksmontville and lucasxsong and removed request for ksmontville March 8, 2024 19:08
Copy link
Contributor

@richford richford left a comment

Choose a reason for hiding this comment

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

Code review and testing of the PR preview link look good! Could I ask you to add or modify copies tests as well? I realize that the cypress test user will not be able to log in with email link. But maybe we could just write a test that toggles the password box and verifies that it is enabled/disabled.

Copy link

cypress bot commented Mar 8, 2024

2 failed tests on run #465 ↗︎

2 24 0 0 Flakiness 0

Details:

Tests for PR 412 "Allow Email login" from commit "92bbe5ef06bd8b352caee070211523...
Project: roar-dashboard-e2e Commit: 92bbe5ef06
Status: Failed Duration: 21:14 💡
Started: Mar 8, 2024 11:13 PM Ended: Mar 8, 2024 11:34 PM
Failed  cypress/e2e/participant/default-tests/playSWR.cy.js • 1 failed test

View Output

Test Artifacts
Testing playthrough of SWR as a participant > ROAR-Word Playthrough Test Test Replay Screenshots
Failed  cypress/e2e/partner-admin/default-tests/viewIndividualReport.cy.js • 1 failed test

View Output

Test Artifacts
The partner admin can view score reports for a given administration. > Selects an administration and views its score report. Test Replay Screenshots

Review all test suite changes for PR #412 ↗︎

Copy link
Collaborator

@lucasxsong lucasxsong left a comment

Choose a reason for hiding this comment

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

This looks really good! I was testing on staging and noticed that this error prevented me from authing with email, but pulling down the changes and running the login flow on localhost worked.

image

I had a small design suggestion: would it be possible to style the disabled password input box to look more like the disabled example in the Primevue docs? The current disabled input box works well in dissuading users from trying to enter a password, but I feel that the visual hinting provided by the darkened input background might help users in quickly identifying what mode they are in (email/pw or email/link). What do you think?

image

@kellyel
Copy link
Collaborator Author

kellyel commented Mar 8, 2024

Thanks for the review, Lucas.
Yes, I noticed that too. However that seems to be the case on all staging links right now. It seems firebase does not recognize the staging links as a valid uri. I just don't think that's been set up yet. It might be possible given that we use the firebase hosting github action to generate these.

Yes, I agree with you. I can darken that background right now.

@richford richford merged commit 7d5116b into main Mar 11, 2024
10 of 14 checks passed
@richford richford deleted the enh/email-login branch March 11, 2024 18:09
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