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 review photo carousel functionality #375

Merged
merged 10 commits into from
Dec 4, 2024
Merged

Add review photo carousel functionality #375

merged 10 commits into from
Dec 4, 2024

Conversation

kea-roy
Copy link
Member

@kea-roy kea-roy commented Oct 24, 2024

Summary

The photo previews in reviews are small, making it difficult to check if a photo is appropriate as an admin and difficult for users to view. This pull request extends the usage of the PhotoCarousel component to support review photo previews and different starting indexes.

  • Refactor the PhotoCarousel component to update the styling to handle large photos.
  • Add functionality to the PhotoCarousel component to display a carousel with a different start index
  • The PhotoCarousel component now accepts a start index as a prop.
  • Clicking on a photo in the Review and AdminReview components triggers the photo carousel with the corresponding photos and start index.
  • Added hover effects and cursor adjustment to indicate the photos are expandable.
  • Added documentation for PhotoCarousel and showPhotoCarousel
  • Created usePhotoCarousel with functions and states to avoid code duplication.
  • Fixed styling issues with the photo carousel.
  • Improved loading speeds when processing large images by using lazy loading
  • Fixed onClose functionality to close the modal

Test Plan

I tested the review pop up on all pages with review components or admin review components, including AdminPage, ApartmentPage, BookmarksPage, LandlordPage, ProfilePage. I tested it on large vertical and horizontal photos to make sure buttons remain on the screen. I also tested responsiveness across mobile and desktop devices. I also tested the photo carousel still works on previewing apartment photos to ensure backward compatibility.

Desktop
Screenshot 2024-10-24 at 11 10 21 AM
Mobile
Screenshot 2024-10-24 at 11 10 51 AM
Desktop
Screenshot 2024-10-24 at 11 11 16 AM
Mobile
Screenshot 2024-10-24 at 11 11 35 AM

Notes

  • Hover effects can be discussed in more detail. Currently, it expands in size and darkens.

- Refactor the PhotoCarousel component to update the styling to handle large photos.
- Add functionality to the PhotoCarousel component to display a carousel with a different start index
- The PhotoCarousel component now accepts a start index as a prop.
- Clicking on a photo in the Review and AdminReview components triggers the photo carousel with the corresponding photos and start index.
@dti-github-bot
Copy link
Member

dti-github-bot commented Oct 24, 2024

[diff-counting] Significant lines: 262.

@kea-roy kea-roy changed the title [WIP] Add review photo carousel functionality Add review photo carousel functionality Oct 26, 2024
@kea-roy kea-roy marked this pull request as ready for review October 26, 2024 18:10
Copy link
Contributor

@CasperL1218 CasperL1218 left a comment

Choose a reason for hiding this comment

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

PR Review

The refactoring of the component performs greatly and it is well handled across multiple different file/page locations. Some suggestions:

  1. The line of dot circles below the photo carousel is a great indication of the display progress. Currently the circles will follow the height of the image thus have some vertical translations. This situation is more visible for photos of significant size differences.
Screenshot 2024-10-27 at 22 31 55 --> Large photo. The display is really nice. Screenshot 2024-10-27 at 22 31 46 --> Tiny photo. The dot circles move vertically upward adjusting to the size of the photo. This could be a design choice, but this could create a jumping effect. Fixed positioning provides a predictable, stable interface element that users can rely on. Users don't have to "chase" the dots as they move up and down, which might help reduce some cognitive load.
  1. The modal carousel's interaction pattern could be refined - currently, users need to move their cursor beyond the navigation arrows to exit the view. While this prevents accidental closures, it may not align with common user expectations, where clicking any area outside the main image content typically dismisses a modal. Consider adjusting the active zone to create a more fluid browsing experience.

@ggsawatyanon ggsawatyanon self-requested a review October 28, 2024 03:53
@kea-roy
Copy link
Member Author

kea-roy commented Nov 6, 2024

PR Changes

  • Fixed styling issues with the photo carousel.
  • Improved loading speeds when processing large images by using lazy loading
  • Fixed onClose functionality to close the modal

Notes

  • While the styling is meant to match Figma as closely as possible, I added a low transparency background on the images to indicate where transparent image borders are located so that users don't feel confused when they click the background of a transparent image and it doesn't close the modal.

@kea-roy kea-roy dismissed CasperL1218’s stale review November 6, 2024 05:12

Resolved suggested changes

Copy link
Contributor

@ggsawatyanon ggsawatyanon left a comment

Choose a reason for hiding this comment

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

This PR is super useful so users can see pictures in bigger resolutions. Thanks for coordinating with designers to get their response!

@ggsawatyanon ggsawatyanon merged commit 3ac2293 into main Dec 4, 2024
4 checks passed
@ggsawatyanon ggsawatyanon deleted the review-carousel branch December 4, 2024 00:32
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.

4 participants