-
Notifications
You must be signed in to change notification settings - Fork 1
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
Build a basic Sign In page #80
Conversation
…ect to /query from signin page button click
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm! Thanks so much for putting this together so quickly
Worth tagging Michelle as well for a design review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! one open q, but no other issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! only blocking because I think as team we need to evaluate our use of "use client"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, and wow y'all work fast!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unblocking this PR. Nice work @katyasoup!
We can continue discussing SSR vs CSR in Slack.
Thanks @katyasoup ! This looks great. Could you please make these changes?
I was trying to review the error state, and it seems like I am able to log in with any credential. Is that just because it doesn't work yet? Is that what this means?
Also, log out takes me to 404. Also not sure if that's intentional. |
@mikang thanks! I'll add in the error state styles (but you are correct, that line about redirecting to /query just means any (or no) credential will work). For the breakpoints - happy to add those here, though I wonder if it's worth capturing it in a new ticket? There are other areas that could use some adjustment as well to be more responsive. Can bring this up at standup, too :) |
…uery-connector into kcd/77-login-page
Awesome @katyasoup !!! The responsiveness is great, and I also see the error states. Could you please make these changes?
Could you also please change the font? This is my fault — I didn't update the designs, sorry! I believe @fzhao99 may have already standardized this in your design system. I've updated the Figma file accordingly.
So it should look like this now: Thank you!! |
I can handle this piece once this PR goes in! We might even get it for free because of the way the styles are tokenized |
Woohoo!!! Awesome, thank you @fzhao99 !!! |
PULL REQUEST
Summary
Related Issue
Fixes #77
Acceptance Criteria
GIVEN the Query Connector landing page
WHEN a user clicks "Sign In"
THEN they are redirected to the /signin page
GIVEN the Query Connector /signin page
WHEN a user clicks Sign In
THEN they are redirected to /query
Note for designers: The data usage policy button wasn't included in the Figma mockup; happy to adjust its placement as needed/desired!
Additional Information
Checklist