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(web): grid on people page #5640

Merged
merged 8 commits into from
Jan 9, 2024
Merged

fix(web): grid on people page #5640

merged 8 commits into from
Jan 9, 2024

Conversation

martabal
Copy link
Member

Changes made in this PR

With this PR, the people grid always takes up the full available width and the size of the thumbnails changes depending on the screen size.

Screenshots

Before After
2023-12-11.21-23-06.mp4
2023-12-11.21-21-13.mp4

Copy link

cloudflare-pages bot commented Dec 13, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4db5a0d
Status: ✅  Deploy successful!
Preview URL: https://d7816658.immich.pages.dev
Branch Preview URL: https://fix-grid-people-page.immich.pages.dev

View logs

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

This seems OK, but you can actually accomplish all of this with css using flex, not with javascript that has to run in the browser and may cause performance issues on slower devices, like phones.

@martabal
Copy link
Member Author

This seems OK, but you can actually accomplish all of this with css using flex, not with javascript that has to run in the browser and may cause performance issues on slower devices, like phones.

The thumbnail width is re-calculated only when the width of the div containing all the thumbnails change. So could be an issue only on screen rotation

@martabal
Copy link
Member Author

martabal commented Jan 9, 2024

Changed to use grid instead

@martabal martabal requested a review from jrasm91 January 9, 2024 01:12
@jrasm91
Copy link
Contributor

jrasm91 commented Jan 9, 2024

It's the video updated too or is that the old one?

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Lgtm

@martabal
Copy link
Member Author

martabal commented Jan 9, 2024

It's the video updated too or is that the old one?

I just changed it

@jrasm91 jrasm91 merged commit 29b204d into main Jan 9, 2024
20 checks passed
@jrasm91 jrasm91 deleted the fix/grid-people-page branch January 9, 2024 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants