Add profile settings menu - Githubissues

ufosc / OSC-Proximity-Chat-App

Talk to the people closest to you. An open source React Native app that combines location and communication into one.

https://osc-proximity-documentation.vercel.app/

GNU General Public License v3.0

43 stars 100 forks source link

Add profile settings menu #265

Closed Chuporceeta closed 3 weeks ago

Chuporceeta commented 3 weeks ago

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.

h1divp commented 3 weeks ago

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.

h1divp commented 3 weeks ago

Thank you for working on this issue!

h1divp commented 3 weeks ago

After reviewing the UI I have a few comments Likes:

The profile picture grid is done nicely and I think the size is very appropriate
I like the position of the profile picture in the top right of the settings menu and how it changes instantly.
Yay we have a menu!!!

What I would improve:

I think I would like it much more if we didn't have the models, and rather just had a text inputs in the settings menu in its own section.
Text inputs need to have a default placeholder text that doesn't affect the default state of what would actually be submitted. That's not saying that there should be no default state for them at all (default color in color one, default name in the other), but when I start to type in them I shouldn't have to delete what's already there.
I think the default color should be what's already used on the buttons and switches instead of the blue

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.

h1divp commented 3 weeks ago

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.

h1divp commented 3 weeks ago

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.