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: fix avatar size constant #247

Merged
merged 4 commits into from
Jun 10, 2024
Merged

fix: fix avatar size constant #247

merged 4 commits into from
Jun 10, 2024

Conversation

mariembencheikh
Copy link
Contributor

@mariembencheikh mariembencheikh commented Jun 5, 2024

fix #246

Copy link
Member

@spaenleh spaenleh left a comment

Choose a reason for hiding this comment

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

Thank you for the quick fix, it solves the issue.

However I think this only fixes the surface of the problem and the core can still be subject to breakage when changing/refactoring the code. Let me explain:

  • There are 3 constants in the app that somehow relate to sizing the big profile picture:
    • AVATAR_SIZE (previously 256, now 200)
    • THUMBNAIL_SETTING_MAX_HEIGHT/WIDTH (200)
    • THUMBNAIL_DIMENSION (200)
  • The AvatarUploader component allows to set avatarSize via an optional prop. This component is used only once, and is passed a prop value for avatarSize coming from the AVATAR_SIZE constant, so the THUMBNAIL_DIMENSION is actually never used.
  • Different parts of the AvatarUploader UI are sized with different constants, it works if they have the same value, but breaks if one of them is changed.

In the light of these elements can you reconsider the fix and make it less fragile to changes. I think reducing the number or constants that affect the same values should be considered as well as ensuring that the elements that should be the same size depend on the same variable instead of different ones that hold the same value.

Let me know if something is not clear. Also please take this as a nitpicking comment from my side, that tries to improve the current code, instead of duck taping it 😄

@mariembencheikh mariembencheikh requested a review from spaenleh June 6, 2024 14:07
Copy link
Member

@spaenleh spaenleh left a comment

Choose a reason for hiding this comment

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

Nice !

@spaenleh spaenleh force-pushed the 246-fix-avatar-size branch from 01ffd55 to efc62ae Compare June 10, 2024 06:35
@spaenleh spaenleh enabled auto-merge June 10, 2024 06:35
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@spaenleh spaenleh added this pull request to the merge queue Jun 10, 2024
Merged via the queue into main with commit 46bd04f Jun 10, 2024
5 checks passed
@spaenleh spaenleh deleted the 246-fix-avatar-size branch June 26, 2024 06:33
spaenleh pushed a commit that referenced this pull request Nov 14, 2024
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
spaenleh pushed a commit that referenced this pull request Dec 16, 2024
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix avatar size
2 participants