-
Notifications
You must be signed in to change notification settings - Fork 1
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
Conversation
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 |
Added issue #36 so that any current issues involving this image carousel may be addressed |
…sd-website into feature/image-carousel
There was a problem hiding this 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.
Looks good. I made one last-minute change: I found out that if you add the |
I didn't know about |
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 .