-
Notifications
You must be signed in to change notification settings - Fork 275
feat: Profile Header & User Form #4987
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| UpdateProfileParameters | ||
| >({ | ||
| mutationFn: ({ image, onUpdateSuccess, ...data }) => | ||
| mutationFn: ({ upload, coverUpload, onUpdateSuccess, ...data }) => |
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.
| mutationFn: ({ upload, coverUpload, onUpdateSuccess, ...data }) => | |
| mutationFn: ({ onUpdateSuccess, ...data }) => |
| upload, | ||
| coverUpload, |
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.
| upload, | |
| coverUpload, |
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'm not following here? These are required for the uploads to work 🤔
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.
Yes, they should still be included, we just don't have to destructure them now since the mutation's data shape is the same as what is being spread here. My bad for not giving enough context/details 🙏
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.
Hence why I removed it here too #4987 (comment)
| }, | ||
| }); | ||
| }, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
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.
Why do we have this disabled?
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 am not 100% sure what the reason was behind this, as I copied the code from Header.tsx which was a lot bigger component, and re-adjusted it a bit. I removed the comment and added the openModal dependency, and there didn't seem to be 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.
Just the last few open points, then we should be 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.
🚀 🌔
|
@AmarTrebinjac last two things but approving to unblock: |
Changes
DESIGN
Not currently testable on preview due to backend compat.
If you wish to test locally, you must also switch your backend branch to MI-1067-update-user-profile (remember to run migrations)
missing the job section, but there's a TODO for that, as it depends on the user experience section being implemented:
For now, since this form already exists and has plenty of logic, I did not fully convert it to a zod schema. Instead using the existing error handling and implementing it with react-hook-form, which worked surprisingly well.
New location input in action:
Screen.Recording.2025-10-13.at.17.42.31.mov
Also supports the type field, but not relevant for this form
Additionally I created a new Select component that has a proper hidden input for dropdowns, as well as new image drag and drop handling compatible with react-hook-form
All other inputs have been wrapped with react-hook-form. Currently the ones with existing forms are just named
ControlledExistingInput. Eventually when we move entirely to react-hook-form, we can maybe remove the controlled part, or if anyone has any other naming suggestions, I'm all ears.Events
Did you introduce any new tracking events?
Experiment
Did you introduce any new experiments?
Manual Testing
Caution
Please make sure existing components are not breaking/affected by this PR
Jira ticket
MI-1067
Preview domain
https://mi-1067.preview.app.daily.dev