-
Notifications
You must be signed in to change notification settings - Fork 101
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 profile settings menu #265
Conversation
Clicking on the profile picture opens a new menu to edit display name, profile color, and profile picture. No settings work currently.
Code looks good but I won't be able to test anything for a little while. My only recommendation so far is to, for the profile picture attribute inside of the useState, to make it an integer for an array index, instead of a require statement (in case if someone would like to change the default image or filename without breaking code). Then perhaps when the state is updated, it just stores a different index that references the array of loaded profile images. |
Thank you for working on this issue! |
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 made a few comments on some changes to be made, but overall, nice job!
Move display name and profile color inputs to separate modals built from a generic text input modal, and relocate to app/components/settings/TextInputs.tsx.
After reviewing the UI I have a few comments
What I would improve:
All in all I think the UX would be a lot better if we didn't have people to tap a few times to get what should be done in a single tap. I think it would feel much more professional as well if the above corrections are made. |
However, at its very core, we now have a basic implantation for the profile settings menu with what was required. If you would like, I can approve this PR now and make an issue for improving this menu. Approving this now would allow me to start letting people work on connecting this menu to the backend and fully building out the profile picture/display name feature. But if you think you can make the changes quickly, I can wait a bit to approve it. |
Update on this decision: I say merge this, and if you want to improve the design yourself we can assign you to the issue about it. |
For #259. Adds a profile picture to the settings screen. Clicking on the picture opens/closes a menu to set the display name, profile picture, and profile picture background color.