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

Fix: Ensure minimum dimension of 120x120px for file uploads #4163

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

rumzledz
Copy link
Contributor

@rumzledz rumzledz commented Jan 22, 2025

Description

As discussed with Product, files being uploaded should have a dimension of at least 120x120px.

min-120x120px

Testing

Pika

pikachu-119x119

  1. Generate a create colony URL: node scripts/create-colony-url.js
  2. On the token creation page, upload Pikachu
  3. Verify that:
  • the error message says: "Image dimensions should be at least 120x120px"
  • you hear an audio clip of an angry Pikachu (of course I'm kidding, don't request changes because of this)
  1. Visit the Planex colony: http://localhost:9091/planex/
  2. Bring up the Edit colony details form
  3. Upload Pikachu as the Colony logo
  4. Verify that the error message says: "Image dimensions should be at least 120x120px"

Resolves #3858

@rumzledz rumzledz self-assigned this Jan 22, 2025
@rumzledz rumzledz marked this pull request as ready for review January 22, 2025 15:55
@rumzledz rumzledz requested a review from a team as a code owner January 22, 2025 15:55
@rumzledz rumzledz force-pushed the fix/3858-strict-min-file-size branch from 99d39bd to 7fd6876 Compare January 22, 2025 15:59
@melyndav
Copy link

Hey @rumzledz -

There are a few other areas this PR needs to address, as the 250x250 dimension was used globally across all image uploads. The following sections need to be updated to ensure consistency with a single dimension for all image touchpoints and across all responsive sizes.

1. Profile page in the User Account section:

image image

2. Edit Colony action - change Colony logo modal

image

@rumzledz
Copy link
Contributor Author

Hey @melyndav ! Thanks for visiting this PR 😄 I can confirm that I have updated those areas as well. It is now a minimum of 120x120px across the board 👌

@melyndav
Copy link

Hey @melyndav ! Thanks for visiting this PR 😄 I can confirm that I have updated those areas as well. It is now a minimum of 120x120px across the board 👌

You're a superstar!!! Thank you @rumzledz 🥳

iamsamgibbs
iamsamgibbs previously approved these changes Jan 24, 2025
Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

🎉 Great job Romeo, this is working very nicely everywhere the file upload is used.

Screenshot 2025-01-24 at 10 41 15

Screenshot 2025-01-24 at 10 41 48

Screenshot 2025-01-24 at 10 42 18

Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

SO the size check works as expected, but one thing I noticed is that the "Remove" button doesn't work.

I can "Change" the image, but cannot remove it to get back to the original blockie

Screencast.from.2025-01-24.14-16-51.mp4

@rumzledz rumzledz force-pushed the fix/3858-strict-min-file-size branch from 7fd6876 to 4b2e775 Compare January 24, 2025 19:23
@rumzledz rumzledz marked this pull request as draft January 24, 2025 19:25
@rumzledz rumzledz force-pushed the fix/3858-strict-min-file-size branch from 4b2e775 to f6dcaa5 Compare January 24, 2025 19:37
@rumzledz rumzledz marked this pull request as ready for review January 24, 2025 19:44
@rumzledz rumzledz force-pushed the fix/3858-strict-min-file-size branch from f6dcaa5 to 37e5fe5 Compare January 24, 2025 19:44
@rumzledz
Copy link
Contributor Author

@rdig I was eventually able to understand the issue you found about not being able to remove the avatar. it turned out to be a master bug that didn't have a reason to make itself known... until now 🐞 I've pushed the changes, hopefully that's the last of that!

@rumzledz rumzledz force-pushed the fix/3858-strict-min-file-size branch from 37e5fe5 to 3852524 Compare January 24, 2025 19:48
@rumzledz rumzledz requested review from rdig, iamsamgibbs and a team January 24, 2025 19:52
@rumzledz rumzledz force-pushed the fix/3858-strict-min-file-size branch from 3852524 to 9a72828 Compare January 25, 2025 09:54
Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Nice job, I can remove the image without issue:
https://github.com/user-attachments/assets/95f4ea1c-5bd6-4b71-803d-e63a3e4b023f

And to confirm, it is created with the blockie:

Screenshot 2025-01-28 at 18 27 08

Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

Approving now, all good!

Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Hey @rumzledz nice work on this issue! 🎉

The right error message is shown if the dimensions are not 120x120px for the token, the colony and the profile avatar image upload

Screen.Recording.2025-01-29.at.10.12.04.mov
Screen.Recording.2025-01-29.at.10.12.40.mov
Screen.Recording.2025-01-29.at.10.14.03.mov

However @rumzledz I did notice on the avatar image upload, once I remove the file with which I wanted to replace it due to it not respecting the dimensions, the current avatar was replaced with a blockie. Is this as intended? If so, I'll happily approve your PR 🙌

Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Approving as the bug related to removing an image from the uploader replacing the existing image with a blockie will be treated in a separate issue. Nice work @rumzledz 🥇

@rumzledz rumzledz merged commit 623b71a into master Jan 29, 2025
2 checks passed
@rumzledz rumzledz deleted the fix/3858-strict-min-file-size branch January 29, 2025 09:42
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.

[Manage Account] Avatar Uploader Accepts Files Smaller Than 250x250px
5 participants