-
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
Feat/login page #86
Feat/login page #86
Conversation
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.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
prettier
apps/site/src/components/ui/form.tsx|127|
apps/site/src/components/ui/form.tsx|130|
apps/site/src/components/ui/form.tsx|144|
apps/site/src/components/ui/form.tsx|147|
apps/site/src/components/ui/form.tsx|168|
apps/site/src/components/ui/input.tsx|1|
apps/site/src/components/ui/input.tsx|3|
apps/site/src/components/ui/input.tsx|6|
apps/site/src/components/ui/input.tsx|9|
apps/site/src/components/ui/input.tsx|25|
apps/site/src/components/ui/label.tsx|1|
apps/site/src/components/ui/label.tsx|3|
apps/site/src/components/ui/label.tsx|7|
apps/site/src/components/ui/label.tsx|10|
apps/site/src/components/ui/label.tsx|14|
apps/site/src/components/ui/label.tsx|18|
apps/site/src/components/ui/label.tsx|26|
Also, any reason why the button is light blue? |
Will mobile responsiveness be in a future issue? |
I just made it blue because. I can change it if you would like |
Is it not responsive? I thought I made it so. If not send a screenshot I'll fix it |
Probably the gold button for consistency Oh it's not a white background...I don't have a better suggestion, that's a graphics thing 😢 |
On second thought, can you use white background? At least that's what the Figma indicates |
Yea in my personal opinion I didn't like the color scheme on the Figma design. Mine isn't great either, but I just wanted to get functionality working first and then change colors later. We can talk to graphics about it. |
It looks like this includes the commits from PR #80 . Wouldn't it be better to only include the shadcn dependencies? |
Agree with Albert here, if we need to set up shadcn first, let's have that in a separate PR and contain the scope of subsequent PRs. |
Yea thats what #80 is supposed to be. |
I think what they're saying is that we should fix up and merge 80 before we work on this because 80 isn't done yet. I'm assuming you based this PR off of 80? |
We need the login page long before the schedule, so we should extract what's needed for both and review that before reviewing the login page design. |
Yea I did because 80 is where I set up shadcn. And since it was my first time using shadcn, I POCed it by doing a quick mock up of the schedule page using shadcn. Because shadcn, you install only the components you need, thru the CLI, so I wanted to test the full process of installing the components and using them. I set up and tested the library to make sure shadcn was viable enough to be used. |
Can you extract only the stuff we need for the login page (shadcn and components) and then keep it here instead of basing this PR off of #80 ? |
This reverts commit 4fc871e.
We are missing the schedule page now. Could you rebase/cherry pick only the relevant changes as originally suggested? |
Closing this as completed by #114 . |
Overview
Login page for hackers to login and apply for IrvineHacks. For Issue #81. You do not need to review the components in the
components/ui
folder of the NextJS site project folder. These are autogenerated shadcn components that were installed. Refer to the docs for more info: https://ui.shadcn.com/docs.