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.

Sign up for GitHub

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

Jump to bottom

fix: added possibility to clear profile & cover photos #3734

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: added possibility to clear profile & cover photos #3734

wants to merge 6 commits into from

Conversation

n-d-r-d-g
Copy link

@n-d-r-d-g n-d-r-d-g commented Oct 30, 2024 •
edited
Loading

Fixes dailydotdev/daily#1534.

Changes

I've added a 'X' button at the top right corner of the profile and cover images at /account/profile to enable the user to clear the image.

image

Please note that I have not been able to test this functionality completely on Gitpod due to an unexpected error when uploading the cover image. The images are cleared on the web app. Not sure if the API call is working fine.

@n-d-r-d-g n-d-r-d-g requested a review from a team as a code owner October 30, 2024 12:19
@vercel Vercel
Copy link

vercel bot commented Oct 30, 2024

@n-d-r-d-g is attempting to deploy a commit to the Daily Dev Team on Vercel.

A member of the Team first needs to authorize it.

@CLAassistant
Copy link

CLAassistant commented Oct 30, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@vercel Vercel
Copy link

vercel bot commented Nov 4, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Nov 4, 2024 11:07am

sshanzel
sshanzel reviewed Nov 4, 2024
View reviewed changes
Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Looks amazing! I think there is also one thing we should update, the part where we early return if the passed value is null here:

https://github.com/dailydotdev/apps/pull/3734/files#diff-20f9e8e03612b5e0ab59d33e6321d9d0df729873044f65e2cb7571629cf26dc4R133-R135

@n-d-r-d-g
Copy link
Author

n-d-r-d-g commented Nov 4, 2024 •
edited
Loading

@sshanzel Makes sense. I understand that the early return needs to be removed. Is that right?

Btw, I've added a background on the profile picture to fix an issue I found:
Whenever there's no profile picture but a cover image is present, the profile picture's placeholder is transparent, hence is not the most obvious in terms of separation between the two images:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sshanzel sshanzel sshanzel left review comments

@rebelchris rebelchris Awaiting requested review from rebelchris rebelchris is a code owner automatically assigned from dailydotdev/web-team

@omBratteng omBratteng Awaiting requested review from omBratteng omBratteng is a code owner automatically assigned from dailydotdev/web-team

@ilasw ilasw Awaiting requested review from ilasw ilasw is a code owner automatically assigned from dailydotdev/web-team

@capJavert capJavert Awaiting requested review from capJavert capJavert is a code owner automatically assigned from dailydotdev/web-team

@nensidosari nensidosari Awaiting requested review from nensidosari nensidosari is a code owner automatically assigned from dailydotdev/web-team

@AmarTrebinjac AmarTrebinjac Awaiting requested review from AmarTrebinjac AmarTrebinjac is a code owner automatically assigned from dailydotdev/web-team

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fix: removal of cover image
4 participants
@n-d-r-d-g @CLAassistant @sshanzel @rebelchris