-
Notifications
You must be signed in to change notification settings - Fork 0
Fixes for image sizes #74
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,16 +2,25 @@ import type { UploadcareImage } from '@prezly/uploadcare'; | |||||
import classNames from 'classnames'; | ||||||
import type { CSSProperties } from 'react'; | ||||||
|
||||||
import { convertSizesToWidths } from './lib'; | ||||||
|
||||||
import './Media.scss'; | ||||||
|
||||||
interface Props { | ||||||
className?: string; | ||||||
image: UploadcareImage; | ||||||
style?: CSSProperties; | ||||||
title?: string; | ||||||
sizes?: string; | ||||||
} | ||||||
|
||||||
export function Media({ className, image, style, title }: Props) { | ||||||
export function Media({ | ||||||
className, | ||||||
image, | ||||||
style, | ||||||
title, | ||||||
sizes = '(max-width: 992px) 800px, (max-width: 576px) 400px, 1200px', | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the order correct here? Should it not go from the lowest to the highest max-width values?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if the order actually matters here, couldn't find anything pointing to that 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've tested this in JSFiddle, and it seems they have to follow the lowest-to-highest order. Otherwise the smaller breakpoint following a larger one will be ignored. Compare these two: |
||||||
}: Props) { | ||||||
const computedClassName = classNames('prezly-slate-media', className, { | ||||||
'prezly-slate-media--image': !image.isGif(), | ||||||
'prezly-slate-media--video': image.isGif(), | ||||||
|
@@ -49,10 +58,13 @@ export function Media({ className, image, style, title }: Props) { | |||||
alt={title} | ||||||
className={computedClassName} | ||||||
src={image.format().cdnUrl} | ||||||
srcSet={[image.srcSet(1200), image.srcSet(800), image.srcSet(400)] | ||||||
srcSet={convertSizesToWidths(sizes) | ||||||
.map((width) => image.srcSet(width)) | ||||||
.filter(Boolean) | ||||||
.join(', ')} | ||||||
sizes={`(max-width: 992px) 800px, (max-width: 576px) 400px, 1200px`} | ||||||
sizes={sizes} | ||||||
width={image.originalWidth} | ||||||
height={image.originalHeight} | ||||||
style={style} | ||||||
title={title} | ||||||
/> | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import { convertSizesToWidths } from './convertSizesToWidths'; | ||
|
||
describe('convertSizesToWidths', () => { | ||
it('Returns empty array when `sizes` does not contain valid size defintions', () => { | ||
expect(convertSizesToWidths('')).toEqual([]); | ||
expect(convertSizesToWidths('test string 123')).toEqual([]); | ||
expect(convertSizesToWidths('test, 123')).toEqual([]); | ||
Comment on lines
+6
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it maybe throw instead? Just to not cover possible human mistakes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I agree it makes sense to throw 👍🏻 |
||
}); | ||
|
||
it('Returns array of width numbers when `sizes` is a valid size definition', () => { | ||
expect( | ||
convertSizesToWidths('(max-width: 992px) 800px, (max-width: 576px) 400px, 1200px'), | ||
).toEqual([800, 400, 1200]); | ||
expect( | ||
convertSizesToWidths('(max-width: 992px) 800w, (max-width: 576px) 400w, 100vw'), | ||
).toEqual([800, 400]); | ||
}); | ||
|
||
it('Returns array of width numbers when `sizes` has just one size definition', () => { | ||
expect(convertSizesToWidths('1200px')).toEqual([1200]); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
const SIZE_DEFINITION_REGEX = /(?:\((?:min|max)-width: \d+px\) ?)?(\d+)(?:px|w)/g; | ||
|
||
export function convertSizesToWidths(sizesString: string): number[] { | ||
const matches = Array.from(sizesString.matchAll(SIZE_DEFINITION_REGEX)); | ||
|
||
return matches.map((match) => parseInt(match[1], 10)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { convertSizesToWidths } from './convertSizesToWidths'; |
Uh oh!
There was an error while loading. Please reload this page.