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.

Sign up for GitHub

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

feat: Restrict using Image Slider to landscape images #1743

Closed
wants to merge 5 commits into from
Closed

feat: Restrict using Image Slider to landscape images #1743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6248363
Excluding ratios
dlnr Nov 5, 2024
05fa52a
Update packages/react/src/ImageSlider/ImageSlider.tsx
dlnr Nov 5, 2024
9190e50
Merge branch 'develop' into hotfix/image-slider-aspect-ratios
VincentSmedinga Nov 5, 2024
a4d9306
Restore support for tall images
VincentSmedinga Nov 5, 2024
2a2c373
Update documentation
VincentSmedinga Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/css/src/components/image-slider/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ The images do not slide automatically.
- Use this for a series of images that belong together.
- Provide at least 2 images and at most 5.
- Feature the most essential image first.
- All images must have the same aspect ratio.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want specify this as a guideline? This isn't currently the case, right? I kind of like that you can use different aspect ratio's in the same slider

Note that very wide or tall aspect ratios are not supported.
- Assume that some or many users will not navigate between the slides and only see the first image.
Display all images separately if you want each of them to receive attention.
- A full-width Image Slider should run from one edge of the Screen to the other;
Expand Down
6 changes: 3 additions & 3 deletions packages/react/src/ImageSlider/ImageSlider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ import { ImageSliderThumbnails } from './ImageSliderThumbnails'
import { Ratio } from '../AspectRatio'
import { Image, ImageProps } from '../Image/Image'

export type ImageSliderImageProps = ImageProps & {
/** Specify the aspect ratio to use for the images. */
aspectRatio: Ratio
export type ImageSliderImageProps = Omit<ImageProps, 'aspectRatio'> & {
/** The aspect ratio to use for the images. Options: `tall`, `square`, `wide`, and `x-wide`. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to specify the options in text here? The type will already show you the possible options, no?

aspectRatio: Pick<Ratio, 'tall' | 'square' | 'wide' | 'x-wide'>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have to use Extract here:

Suggested change
aspectRatio: Pick<Ratio, 'tall' | 'square' | 'wide' | 'x-wide'>
aspectRatio: Extract<Ratio, 'tall' | 'square' | 'wide' | 'x-wide'>

}

export type ImageSliderProps = {
Expand Down
Loading