-
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
Add preliminary Apply page and application form #63
Conversation
Deploy preview for irvinehacks-site-2024 ready!
|
Deploy preview for irvinehacks-site-2024-sanity ready!
|
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.
Thanks for getting started on this! I've left a lot of comments, so please let me know if you have any questions on them.
Additionally, I'm not sure what could be happening but the code doesn't seem to be formatted. Is Prettier formatting on save?
Offnote: I feel a component library might have helped here. But it is already built so moot point. |
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.
- Blocking: we could create components for each of the repeatable parts of the form (dropdowns, textfields, paragraph blocks, etc.). It would make the code more DRY and less boilerplatey
- suggestion: I feel the color theme for the app form is not that good. I personally don't like the white background color. Maybe a different color theme would be better to fit our "Night on Lunar New Year" cc: Graphics
- suggestion: Experiment with making the Submit Application button pop out more (maybe make it glow more or smth cause I feel it is lackluster)
- suggestion: I still think "Apply" looks incredibly jarring. Like it kinda scares my eyes away from it. cc: Graphics
Why didn't we use a component library here? The form is pretty much unstyled anyway, so using prebuilt components wouldn't be a downside.
Example: https://mui.com/material-ui/react-radio-button/
I should have suggested the use of Radix in this case.
I should have suggested the use of Radix here. I'm slightly concerned about installing Material just so we can use radio buttons. We can transition to using components in a separate issue. For now, @Bl20052005 , please resolve the issues Tyler brought up, including extracting out the components so that you don't repeat yourself too much, and then re-request reviews from us. |
Material has other components as well textfields, dropdowns, etc. Tho I agree adding another component library is too much. I think they allow you to only import certain components, not sure if it is worth looking into tho. A possible alternative or improvement is to use shadcn. You use a CLI tool to download only the components you need, so it is lightweight. Plus it is built on Radix and Tailwind which we use already. Tradeoff is learning it, but it doesn't look too hard. It is basically just templates of Radix Primitives. |
Yeah @Bl20052005, as both @taesungh and @tyleryy have said, we don't need the username or password section because they'll be logging in before accessing the application |
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.
Sorry, a lot of these comments should've been reviewed among log before sending over to tech. Thanks Bo for the awesome job so far!
Question: should we be using sanity for this page? Or are we hardcoding it? That way Log can make changes in realtime / on the fly. |
We're just hard coding it since the questions have already been finalized. |
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.
Two more typo changes, otherwise looks good from log!
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.
Let's call it here, we can fix smaller lingering issues separately.
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.
Agreed, we can fix any lingering issues later. Will be squash merging.
initial pr for application setup, still some work to do