Skip to content
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

Merged
merged 9 commits into from
Sep 13, 2024
Merged

Conversation

Chuporceeta
Copy link
Contributor

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.

Clicking on the profile picture opens a new menu to edit display name, profile color, and profile picture. No settings work currently.
@h1divp
Copy link
Collaborator

h1divp commented Sep 11, 2024

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
Copy link
Collaborator

h1divp commented Sep 11, 2024

Thank you for working on this issue!

Copy link
Collaborator

@dyland88 dyland88 left a 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!

client/app/screens/settings/SettingsScreen.tsx Outdated Show resolved Hide resolved
client/app/screens/settings/SettingsScreen.tsx Outdated Show resolved Hide resolved
client/app/screens/settings/SettingsScreen.tsx Outdated Show resolved Hide resolved
client/app/screens/settings/SettingsScreen.tsx Outdated Show resolved Hide resolved
dyland88 and others added 4 commits September 12, 2024 11:56
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.
@h1divp
Copy link
Collaborator

h1divp commented Sep 13, 2024

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
Copy link
Collaborator

h1divp commented Sep 13, 2024

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
Copy link
Collaborator

h1divp commented Sep 13, 2024

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.

@dyland88 dyland88 merged commit 13e52aa into ufosc:main Sep 13, 2024
2 of 3 checks passed
@Chuporceeta Chuporceeta deleted the add-profile-settings-menu branch October 2, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants