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 user profile picture #4431

Merged
merged 1 commit into from May 7, 2020
Merged

Conversation

OmeGak
Copy link
Member

@OmeGak OmeGak commented Apr 29, 2020

Partially addresses #936.

This PR adds:

  • A file upload field in the user personal details form.
  • Display of the profile picture in the user profile dashboard.

Functional testing instructions

Upload profile pic:

  1. Go to user profile settings (user/<user_id>/profile/)
  2. Upload a file from your computer
  3. Verify that the picture is uploaded successfully
  4. Verify that the profile picture is displayed in the user profile dashboard (user/<user_id>/dashboard/)

Delete profile pic:

  1. Go to user profile settings (user/<user_id>/profile/)
  2. Delete the profile picture
  3. Verify that the profile picture is not displayed in the user profile dashboard (user/<user_id>/dashboard/)

Screenshots

image

image

image

@OmeGak OmeGak force-pushed the wip/profile-pic branch 2 times, most recently from 8a7309e to 4f12d13 Compare Apr 29, 2020
@OmeGak OmeGak marked this pull request as ready for review Apr 30, 2020
@OmeGak OmeGak force-pushed the wip/profile-pic branch 2 times, most recently from 6debe8c to 8acea4f Compare May 4, 2020
indico/modules/users/controllers.py Outdated Show resolved Hide resolved
indico/modules/users/controllers.py Outdated Show resolved Hide resolved
indico/modules/users/controllers.py Outdated Show resolved Hide resolved
indico/modules/users/controllers.py Outdated Show resolved Hide resolved
indico/modules/users/models/users.py Outdated Show resolved Hide resolved
indico/modules/users/templates/personal_data.html Outdated Show resolved Hide resolved
indico/modules/users/controllers.py Outdated Show resolved Hide resolved
indico/modules/users/controllers.py Outdated Show resolved Hide resolved
indico/web/client/styles/modules/_dashboard.scss Outdated Show resolved Hide resolved
@OmeGak OmeGak force-pushed the wip/profile-pic branch 10 times, most recently from b37801d to eb6a030 Compare May 5, 2020
@ThiefMaster ThiefMaster requested a review from pferreir May 6, 2020
@ThiefMaster
Copy link
Member

ThiefMaster commented May 6, 2020

When testing with my octocat avatar it somehow got completely destroyed during the conversion. Not sure why this happened though...

It also looks a bit strange right now if the picture is smaller than the circle where it's displayed.


When I tried it with a non-transparent version it worked fine btw and looks great!
image

@ThiefMaster
Copy link
Member

ThiefMaster commented May 6, 2020

Regarding the upload UI, would it make sense to have a separate box for it on the page instead of including it in the "Details" box?

@OmeGak
Copy link
Member Author

OmeGak commented May 6, 2020

When testing with my octocat avatar it somehow got completely destroyed during the conversion. Not sure why this happened though...

I'll look into this. Probably something that I need to pass to PIL to preserve the transparency.

Regarding the upload UI, would it make sense to have a separate box for it on the page instead of including it in the "Details" box?

I've tried a few variations, but I'm not crazy about any of them.

label no label
top Screen Shot 2020-05-06 at 13 31 44 Screen Shot 2020-05-06 at 13 31 33
bottom Screen Shot 2020-05-06 at 13 35 19 Screen Shot 2020-05-06 at 13 35 12

@ThiefMaster
Copy link
Member

ThiefMaster commented May 6, 2020

True. Doesn't look super amazing. Any other ideas here @indico/development-team?

@panagiotappl
Copy link
Member

panagiotappl commented May 6, 2020

I personally like both the top label and bottom label ideas. IMO, they look good. I would probably expect to see that on top rather than the bottom part of the page.

@OmeGak
Copy link
Member Author

OmeGak commented May 6, 2020

I personally like both the top label and bottom label ideas. IMO, they look good. I would probably expect to see that on top rather than the bottom part of the page.

Two concerns:

  • What would you do with the Block User button? I would not leave it on the bottom box, but putting it on the Profile Picture box looks semantically wrong.
  • The label looks redundant, as the box has the same title. We could change the label or box title to something else, but at that point we'll be contorting the UI just for the sake of having two boxes.

Personally, I would just leave it as it is, with a single box.

@panagiotappl
Copy link
Member

panagiotappl commented May 6, 2020

Maybe it's better to leave it in the same box after all. I don't think it looks bad if it's in the same box anyway. I don't see why the picture cannot be in basic details.

@panagiotappl panagiotappl reopened this May 6, 2020
@panagiotappl
Copy link
Member

panagiotappl commented May 6, 2020

I'm sorry. I accidentally clicked "Close and comment" . :(

@OmeGak
Copy link
Member Author

OmeGak commented May 6, 2020

@ThiefMaster, upload of transparent images is now working fine. I was removing the alpha channel. This fixes it:

- if pic.mode != 'RGB':
+ if pic.mode not in {'RGB', 'RGBA'}:
     pic = pic.convert('RGB')

@plourenco
Copy link
Member

plourenco commented May 6, 2020

I like the top options, I'm not sure why but it seems straightforward to have a picture as one of the first details to be edited.

Apart from that, perhaps I could suggest having the image squared (side-by-side) with the details inputs? The same way we're now using in #4408.

@OmeGak OmeGak force-pushed the wip/profile-pic branch 6 times, most recently from 234916b to dbdfd2f Compare May 6, 2020
indico/modules/users/templates/dashboard.html Outdated Show resolved Hide resolved
indico/modules/users/controllers.py Show resolved Hide resolved
indico/modules/users/controllers.py Show resolved Hide resolved
@ThiefMaster ThiefMaster merged commit c112fba into indico:master May 7, 2020
5 checks passed
@OmeGak OmeGak deleted the wip/profile-pic branch Jul 2, 2020
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.

None yet

5 participants