-
Notifications
You must be signed in to change notification settings - Fork 19
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
Implement about page #40
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/acmutd/hackportal/5TLYbnAyfWaxtGBs7KTLRFFGJkzD |
📝 Changed routes:
And 5 other routes:
Commit 0c3b031 (https://hackportal-bvdze3mgb-acmutd.vercel.app). |
I've reviewed the current code and everything looks good. Will come back to the PR once the "Next Steps" have been completed. |
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.
A valiant contribution. This PR is not close to ready for a merge because functionality - especially for the FAQ - has not been implemented. Anything merged into the develop
branch needs to have working functionality, and the contributions far need to be closer to completion to warrant being merged. Additionally, there are some lingering documentation issues that need to be addressed.
I recommend marking this pull request as a draft until you have completed implementation of the features that this PR is supposed to address. |
Moved color schemes list from utilities file to component file to make it local to the component only
Reformat components from anonymous functions to standard named functional component. Also renamed Props interface to use standard component properties name
Moved hardcoded data to lib/data.ts. Setup flow such that hardcoded data can be easily replaced with logic to fetch real data from backend
Added documentation to components created for About section
Here's the error:
It appears that you're explicitly making a request to |
Resolve error regarding attempting to make request to localhost:3000 on staging environment
Resolves warnings came from React when using useEffect hook
When user fails to submit question, appropriate error message will be shown in a user-friendly way.
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.
Vercel-specific issues will be identified by @WillieCubed
Everything else looks good.
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.
This is incredibly Gucci!
For a future release, I will recommend updating the answered questions in realtime by using the relevant Cloud Firestore API or telling the user that they will have to refresh to see the answered questions. However, the only major change to make is showing the expanded answered questions by default as it's not clear to the user they need to click on the answers to expand them as that's an important part of UX that will likely get overlooked by the user.
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!
Changes made in this PR:
/about/questions
.