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

Allow users to clear their profile and header picture #13234

Merged
merged 1 commit into from
Apr 20, 2020
Merged

Allow users to clear their profile and header picture #13234

merged 1 commit into from
Apr 20, 2020

Conversation

sternenseemann
Copy link
Contributor

@sternenseemann sternenseemann commented Mar 9, 2020 •
edited
Loading

This Pull-Request is #8646, but completely reworked.

@sternenseemann sternenseemann mentioned this pull request Mar 9, 2020
Closed
ykzts
ykzts approved these changes Mar 9, 2020
View reviewed changes
Copy link
Member

@ykzts ykzts left a comment

Choose a reason for hiding this comment

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

👍

@ClearlyClaire
Copy link
Contributor

It looks ok to me, however, we have recently merged #13192 which provides something similar for server-wide uploads. It would probably make sense to stick with that design.

Also note that one test fails because it clicks the first button of type submit to submit profile changes, but now, the first submit button deletes the header.

@sternenseemann
Copy link
Contributor Author

sternenseemann commented Mar 31, 2020 •
edited
Loading

I updated the pull request to match the site upload deletion UI!

Screenshot from 2020-03-31 18-57-11

I tried to implement it similarly, but unfortunately I encountered the following problem: @rails/ujs only allows sending a request body when using data-remote="true" which makes the request asynchronous. The problem is that we need to send a PUT request with the body account[subaction]=clear_avatar/header in a synchronous fashion, so the settings page is reloaded and the user can see the changes. To fix this I implemented the missing feature myself, controlled by data-sync-params and data-sync-method in utils/sync-ujs.js. An alternative could be to add new endpoints for deletion (similarly to site uploads). Let me know what you think about my solution!

I still feel like that this should be a part of @rails/ujs, so I made a version of this for it as well and suggested that change. I hope this could be part of rails in the future, so mastodon could drop the own implementation of the feature.

@sternenseemann sternenseemann requested a review from ykzts March 31, 2020 17:19
@ClearlyClaire
Copy link
Contributor

The chunk of JS isn't that big, but wouldn't it be simpler to have separate routes for this instead of having new parameters?

@sternenseemann sternenseemann force-pushed the clear_pictures branch 2 times, most recently from 428e14e to 6e83910 Compare April 6, 2020 13:04
@sternenseemann
Copy link
Contributor Author

sternenseemann commented Apr 6, 2020 •
edited
Loading

I've reworked the pull request completely and dropped the subaction parameter and the UI code completely. Instead of that I now send a simple DELETE request to /settings/profile/pictures/avatar or /settings/profile/pictures/header similar to how it is done for site uploads.

At first I was thinking about adding a /@username/avatar etc. route which would show the avatar for every account and would allow to send a DELETE request for one's own account, but decided to keep everything in /settings for simplicity's sake. If you think that would be nicer, I could still rework it.

PS: Also changed link text from generic.clear to generic.delete so no new translations are needed.

@sternenseemann sternenseemann requested review from Gargron and ykzts April 6, 2020 13:13
ClearlyClaire
ClearlyClaire requested changes Apr 6, 2020
View reviewed changes
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

LGTM overall, except for the part I commented inline

app/helpers/settings_helper.rb Outdated Show resolved Hide resolved
@sternenseemann
Allow users to delete their header and avatar
70561b7 
This is achieved by sending a DELETE request to
/settings/profile/pictures/{avatar,header} via a link that is part of
the upload form's hint of the respective picture.
@sternenseemann
Copy link
Contributor Author

@ykzts @Gargron Is the change alright like this? Anything preventing a merge of this PR?

@Gargron Gargron merged commit 679980f into mastodon:master Apr 20, 2020
@sternenseemann sternenseemann mentioned this pull request Apr 20, 2020
Closed
@ykzts ykzts linked an issue Apr 20, 2020 that may be closed by this pull request
Impossible to erase images in config #4458
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ClearlyClaire ClearlyClaire ClearlyClaire approved these changes

@Gargron Gargron Awaiting requested review from Gargron

@ykzts ykzts Awaiting requested review from ykzts

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

Successfully merging this pull request may close these issues.

Impossible to erase images in config
4 participants
@sternenseemann @ClearlyClaire @ykzts @Gargron