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

Improve photo album pages #5128

Merged
merged 7 commits into from
Nov 6, 2024
Merged

Improve photo album pages #5128

merged 7 commits into from
Nov 6, 2024

Conversation

eikhr
Copy link
Member

@eikhr eikhr commented Oct 30, 2024

Description

The image gallery was a bit clunky and had some bugs.

The following issues are fixed in this PR:

  • Opening and closing an image in an album resets scroll position
  • Photo galleries used an old, unnecessary ProgressiveImage component
  • Going to next and previous image in single photo view in album is sloooow
  • Photos have different heights, making it hard to hit the next/previous buttons multiple times
  • When closing a modal with the button, the element behind the close button is also pressed
  • Photo gallery modal is not dismissable with [esc] or by pressing outside it

Should be tested to really feel the improvements.

Result

The above issues are fixed. Minor visual changes.

  • Changes look good on both light and dark theme.
  • Changes look good with different viewports (mobile, tablet, etc.).
  • Changes look good with slower Internet connections.



Description Before After
Photo modal on mobile

webapp-staging abakus no_photos_423_picture_30553(iPhone 12 Pro)

localhost_3000_photos_427(iPhone 12 Pro)

Photo edit modal (could use some more work...)
It now has a lot less duplicated code, though.

webapp-staging abakus no_photos_423_picture_30553(iPhone 12 Pro) (2)

localhost_3000_photos_427(iPhone 12 Pro) (1)

Testing

  • I have thoroughly tested my changes.

Please describe what and how the changes have been tested, and provide instructions to reproduce if necessary.


Resolves ABA-1159

Copy link

vercel bot commented Oct 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lego-bricks-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 30, 2024 9:41pm

@github-actions github-actions bot added the review-needed Pull requests that need review label Oct 30, 2024
Copy link

linear bot commented Oct 30, 2024

This required adding a ref to the modal and making the dropdown use this
ref as the container. It previously used document.body as the container,
meaning any clicks in the dropdown were counted as outside the modal and
closed the modal.
Copy link
Member

@jonasdeluna jonasdeluna left a comment

Choose a reason for hiding this comment

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

Awesome! If it works it works 👍

Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

This looks much better! 😍 I like

app/components/ProgressiveImage/index.tsx Show resolved Hide resolved
app/components/HeaderNotifications/index.tsx Show resolved Hide resolved
app/actions/GalleryPictureActions.ts Show resolved Hide resolved
@@ -1,14 +1,27 @@
@import url('~app/styles/variables.css');

.loadingIndicator {
margin: 0.375rem;
Copy link
Member

Choose a reason for hiding this comment

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

no

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It avoids a slight movement in the layout, which doesn't look great.

@@ -22,6 +35,9 @@
}

.dropdown {
position: absolute;
top: 0;
right: 3rem;
Copy link
Member

Choose a reason for hiding this comment

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

no

Copy link
Member Author

@eikhr eikhr Nov 5, 2024

Choose a reason for hiding this comment

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

How should we position this? I personally think the current position looks a bit off.
Screenshot 2024-11-05 at 17 09 45
The close-button in the modal is also absolute-positioned, so I don't think there is any other easy way to get the correct positioning.

@ShaileshS1702
Copy link
Contributor

It look so much nicer!

@eikhr eikhr merged commit a7380f6 into master Nov 6, 2024
6 checks passed
@eikhr eikhr deleted the improve-photo-gallery branch November 6, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-needed Pull requests that need review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants