-
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
Edit profile page #91
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
alex feedback
…ship-portal-ui-v2 into sean/edit-profile
Also move Modal to common component
could be useful for the Add to Apple calendar button
…ship-portal-ui-v2 into sean/edit-profile
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.
Functionality works great! Thanks for also refactoring the other modals, EventDetailsForm, and the Add to Apple Calendar button 😁
Some questions/comments I had:
- remove redundant async onChange - reduce code duplication with `hasChange` - move `handleSubmit` out of JSX - rename `updateCurrentUser` to `updateCurrentUserProfile` so it doesn't imply you're switching users - hide heading collapsible arrows from screenreaders - moved icons to SocialMediaIcon component
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 🚢
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.
Everything looks great and functional - couple UI nitpicks.
-
I'm on mobile and a dropdown arrows shows up on both the left and right side of the details tag which seems unintended.
-
Thoughts on "Edit Profile" -> "Manage Account" since not all of the edits on this page are publicly facing.
-
We have a route on the website
/logout
but no links to it anywhere so far. I think this page makes the most sense to include that link since it wouldn't be used too commonly by most users. -
I think we have something similar on some other form components but it would help a lot to highlight which fields have been edited and are currently unsaved so I can know before clicking the cancel or save button and making unintended edits.
Co-authored-by: Faris Ashai <[email protected]>
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.
Hey this looks great! The cropper modal is really well done. Just a few concerns:
Second Faris's fourth point about which fields have been edited. Right now it's a little confusing when changes are actually updated. Certain fields like profile picture and resume are updated at the moment you upload while other fields are only saved when you click save at the bottom. I think it's also a little confusing that pressing enter in one field will attempt to save all of them, including the password. Maybe we should think about moving the reset password to it's own section. Also, editing the email address is a pretty big change since it require's reverifying your email and I wonder if people can get locked out of their accounts by changing it, so maybe that field also needs more warning. Perhaps we can take inspiration from the v1 profile edit page where each section, separated by lines, gets saved together. However, I think we should go ahead and merge this branch and then address those issues in a future PR. |
Your cookie caches your user data locally, so it can get stale when you make changes to your profile (such as earning points) from a different browser or device. We probably should look into better ways to store the user object |
- make placeholder/char count more visible in dark mode - actually update the user's resume visibility preference - update edit profile -> manage account - position sticky the save button when there are unsaved changes - made dark-text-on-bg-2 (the placeholder color) distinct from -bg-1 removing overflow: auto on main was to allow for position: sticky. it seems to have come from the leaderboard PR so whatever
…ship-portal-ui-v2 into sean/edit-profile
- fix iOS Safari double arrow - fix select being unreadable (white on white) in dark mode - ensure `user` object is up to date
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.
I think the reset password form should be separate visually and with its own action button since it's destructive and not the same as updating profile fields. aside from that, this should be fine to merge and iterate later
Info
Closes #87
Description
What changes did you make? List all distinct problems that this PR addresses. Explain any relevant
motivation or context.
Created the edit profile page, including its functionality.
I've also added some new components and utility functions, which would be helpful for other parts of the website as well:
Modal
component for general modals.Cropper
component for cropping user-selected images to a specific aspect ratio. It will also help discourage users from uploading non-images, and it enforces max file sizes by compressing the file as a JPG until it's small enough.useObjectUrl
that creates an object URL for the givenFile
, and revokes it when it's done.accessibly-hidden
mixin for elements that should be tab-accessible but should also pretend to be hidden.Changes
Type of Change
expected)
linting/formatting)
workflows)
Testing
I have tested that my changes fully resolve the linked issue ...
Checklist
src/lib
functions and commented hard to understand areasanywhere else.
Screenshots
Please include a screenshot of your Cypress testing suite passing successfully.
If you made any visual changes to the website, please include relevant screenshots below.