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 image carousel #34

Merged
merged 9 commits into from
Dec 10, 2023
Merged

Add image carousel #34

merged 9 commits into from
Dec 10, 2023

Conversation

Brian-Magnuson
Copy link
Contributor

@Brian-Magnuson Brian-Magnuson commented Nov 18, 2023

Resolves #31. Adds an image carousel to the landing page. The image carousel is from this npm package: https://www.npmjs.com/package/react-responsive-carousel .

image

@Brian-Magnuson
Copy link
Contributor Author

Currently, the image carousel is designed to span the entire width of the screen and be 600px tall. If the image is too big, it is resized via the object-fit: cover CSS property. A limitation I noticed was how right-clicking the image accesses the image behind the carousel, which is currently the background image for the page. Also, while adding the image, I noticed that the background image has a limited height and eventually cuts off revealing the gradient behind.

@Brian-Magnuson Brian-Magnuson marked this pull request as ready for review December 2, 2023 20:04
@Brian-Magnuson
Copy link
Contributor Author

Added issue #36 so that any current issues involving this image carousel may be addressed

@Brian-Magnuson Brian-Magnuson requested a review from a team December 2, 2023 20:17
Copy link
Member

@WillBAnders WillBAnders left a comment

Choose a reason for hiding this comment

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

@Brian-Magnuson Added a few commits to address review comments; most of which we discussed earlier:

  • Rename files to use lowercase extensions (a: consistency, b: matches the name used in the source code). This avoid any risks involve case-sensitivity.
  • Using pointer-events: auto allows the image to be interacted with; so this resolves the concern of the "View full image" piece.
  • Capped the max-width at 100vw / 2 to force a reasonable aspect ratio for the carousel/images. This avoids weird rendering due to the one portrait oriented image. This is far from ideal but I'm inclined to take the solution for now rather than iterate further; can open an issue to revisit it if you're inclined.

If these all seem good for the time being, let's go ahead and merge to close this out. Others can pick up trying to improve this next semester as desired.

Before vs After: Not amazing, but doesn't look (as) weird.
image
image

@Brian-Magnuson
Copy link
Contributor Author

Looks good. I made one last-minute change: I found out that if you add the preventMovementUntilSwipeScrollTolerance prop to the ImageCarousel, it improves scrolling on mobile (Google Chrome). That is, touching the carousel will still allow vertical scrolling on the page.
pointer-events: auto is pretty neat. I never knew that could work.
Will merge shortly. Thank you!

@Brian-Magnuson Brian-Magnuson merged commit 117ec1a into master Dec 10, 2023
1 check passed
@Brian-Magnuson Brian-Magnuson deleted the feature/image-carousel branch December 10, 2023 17:04
@WillBAnders
Copy link
Member

I didn't know about pointer-events: auto either; I was looking through the styles via inspector and found pointer-events: none was being added by the library and disabling that solved the problem.

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.

Add an image carousel to the Landing page
2 participants