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 Pic Removal Button #340

Merged
merged 13 commits into from
Sep 6, 2024
Merged

Add Profile Pic Removal Button #340

merged 13 commits into from
Sep 6, 2024

Conversation

solderq35
Copy link
Contributor

Closes #303

Screenshots

08-28-2024-remove-pfp-1
08-28-2024-remove-pfp-2

Additional Questions

  • Do we need to add a "remove button" for the Team Picture as well? I assumed this was out of scope.

Copy link
Contributor

@FrankreedX FrankreedX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and works! Just polishing now.

app/content/profile/index.js Outdated Show resolved Hide resolved
app/content/profile/index.js Outdated Show resolved Hide resolved
app/content/profile/index.js Outdated Show resolved Hide resolved
@solderq35
Copy link
Contributor Author

solderq35 commented Aug 31, 2024

Updated Screenshots

"Upload" button text (no picture initially)

image

"Modify" button text (there is a picture initially)

image

@FrankreedX
Copy link
Contributor

Hm, if this is how you implemented it, I think it would be better if we just make it a dialog. I like your new styling a lot tho! A dialog that fly up from the bottom is not very consistent with the rest of the dialog in the app

@solderq35
Copy link
Contributor Author

So does this need a cancel button or is it fine to just expect the user can just dismiss the dialog by clicking outside of the dialog?

image

@solderq35
Copy link
Contributor Author

Some Screenshots of New Bottom Modal Implementation

I decided to gray out the "Remove" button when no Profile Picture was present due to a bug:
09-02-2024-obj-not-found-error

  • It could also be better to just hide the button entirely I suppose, although I was concerned the modal looks a bit empty that way

09-02-2024-edit-pfp-ss-1
09-02-2024-edit-pfp-ss-2

Copy link
Contributor

@FrankreedX FrankreedX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there! Looks good

components/bottomSheetWrapper.js Outdated Show resolved Hide resolved
app/content/profile/index.js Outdated Show resolved Hide resolved
@solderq35
Copy link
Contributor Author

solderq35 commented Sep 4, 2024

LGTM (I think I can't technically review my own PR but consider this my review)

image
image

@solderq35 solderq35 merged commit 4ffc0da into main Sep 6, 2024
1 check passed
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.

Add Functionality to Delete Existing Profile Picture
2 participants