-
Notifications
You must be signed in to change notification settings - Fork 5
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
image formatter: choose the smallest thumbnail that is big enough #471
Changes from all commits
da061f1
e6a45ef
49ec003
dabf325
2139a7b
971fd33
0bed343
3b8f175
dc7835d
97d59ab
1e09783
e90a57d
95b0f39
00a5011
0a9cd10
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 |
---|---|---|
|
@@ -331,77 +331,28 @@ export function image(simpleOrComplexImage = {}, size = '200x', atLeastAsLarge = | |
}); | ||
} | ||
|
||
let height, index; | ||
let width = (height = -1); | ||
let desiredWidth, desiredHeight; | ||
let desiredDims = desiredSize.split('x'); | ||
|
||
if (desiredDims[0] !== '') { | ||
width = Number.parseInt(desiredDims[0]); | ||
if (Number.isNaN(width)) { | ||
desiredWidth = Number.parseInt(desiredDims[0]); | ||
if (Number.isNaN(desiredWidth)) { | ||
throw new Error("Invalid width specified"); | ||
} | ||
} | ||
|
||
if (desiredDims[1] !== '') { | ||
height = Number.parseInt(desiredDims[1]); | ||
if (Number.isNaN(height)) { | ||
desiredHeight = Number.parseInt(desiredDims[1]); | ||
if (Number.isNaN(desiredHeight)) { | ||
throw new Error("Invalid height specified"); | ||
} | ||
} | ||
|
||
let widthOk = width === -1; | ||
let heightOk = height === -1; | ||
|
||
if (atLeastAsLarge) { | ||
index = image.thumbnails.length - 1; | ||
|
||
while (index >= 0) { | ||
if (!(image.thumbnails[index].width && image.thumbnails[index].height)) { | ||
return image.thumbnails[index].url; | ||
} | ||
|
||
widthOk = width > 0 ? (image.thumbnails[index].width >= width) : widthOk; | ||
heightOk = height > 0 ? (image.thumbnails[index].height >= height) : heightOk; | ||
|
||
if (heightOk && widthOk) { | ||
break; | ||
} | ||
|
||
index--; | ||
} | ||
|
||
// if we exhausted the list | ||
if (index <= 0) { | ||
index = 0; | ||
} | ||
} else { | ||
index = 0; | ||
|
||
while (index < image.thumbnails.length) { | ||
if (!(image.thumbnails[index].width && image.thumbnails[index].height)) { | ||
return image.thumbnails[index].url; | ||
} | ||
|
||
if (width > 0) { | ||
widthOk = image.thumbnails[index].width <= width; | ||
} | ||
|
||
if (height > 0) { | ||
heightOk = image.thumbnails[index].height <= height; | ||
} | ||
|
||
if (heightOk && widthOk) { break; } | ||
|
||
index++; | ||
} | ||
|
||
// if we exhausted the list | ||
if (index >= image.thumbnails.length) { | ||
index = image.thumbnails.length - 1; | ||
} | ||
} | ||
|
||
return image.thumbnails[index].url; | ||
const thumbnails = image.thumbnails | ||
.filter(thumb => thumb.width && thumb.height) | ||
.sort((a, b) => b.width - a.width); | ||
return atLeastAsLarge | ||
? _getSmallestThumbnailOverThreshold(thumbnails, desiredWidth, desiredHeight) | ||
: _getLargestThumbnailUnderThreshold(thumbnails, desiredWidth, desiredHeight); | ||
} | ||
|
||
const result = imageBySizeEntity(img, size, atLeastAsLarge); | ||
|
@@ -415,6 +366,58 @@ export function image(simpleOrComplexImage = {}, size = '200x', atLeastAsLarge = | |
); | ||
} | ||
|
||
/** | ||
* Gets the smallest thumbnail that is over the min width and min height. | ||
* If no thumbnails are over the given thresholds, will return the closest one. | ||
* | ||
* This method assumes all thumbnails have the same aspect ratio, and that | ||
* thumbnails are sorted in descending size. | ||
* | ||
* @param {Array<{{url: string, width: number, height: number}}>} thumbnails | ||
* @param {number|undefined} minWidth | ||
* @param {number|undefined} minHeight | ||
* @returns {string} | ||
*/ | ||
function _getSmallestThumbnailOverThreshold(thumbnails, minWidth, minHeight) { | ||
let index = thumbnails.length - 1; | ||
while (index > 0) { | ||
const thumb = thumbnails[index]; | ||
const widthOverThreshold = minWidth ? thumb.width >= minWidth : true; | ||
const heightOverThreshold = minHeight ? thumb.height >= minHeight : true; | ||
if (widthOverThreshold && heightOverThreshold) { | ||
return thumb.url | ||
} | ||
index--; | ||
} | ||
return thumbnails[0].url; | ||
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. The JSDoc says this returns the closet thumbnail if one can't be found that exactly meets the criteria. This is the biggest image, right? It might be a bit clearer to simply say we return the largest in the JSDoc. Similarly for 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 was having a really hard time naming this, I guess because the default we're giving is not within the threshold, so I was worried if I didn't have some kind of addendum it would throw people off. I'll remove the IfPossible though, do you think the name is fine after 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 think removing it will make the name better. The current name makes me think the output is optional. We just need to make the default clear in the JSDoc itself. |
||
} | ||
|
||
/** | ||
* Gets the largest thumbnail that is under the max width and max height. | ||
* If no thumbnails are under the given thresholds, will return the closest one. | ||
* | ||
* This method assumes all thumbnails have the same aspect ratio, and that | ||
* thumbnails are sorted in descending size. | ||
* | ||
* @param {Array<{{url: string, width: number, height: number}}>} thumbnails | ||
* @param {number|undefined} maxWidth | ||
* @param {number|undefined} maxHeight | ||
* @returns {string} | ||
*/ | ||
function _getLargestThumbnailUnderThreshold(thumbnails, maxWidth, maxHeight) { | ||
let index = 0; | ||
while (index < thumbnails.length) { | ||
const thumb = thumbnails[index]; | ||
const widthOverThreshold = maxWidth ? thumb.width <= maxWidth : true; | ||
const heightOverThreshold = maxHeight ? thumb.height <= maxHeight : true; | ||
if (widthOverThreshold && heightOverThreshold) { | ||
return thumb.url | ||
} | ||
index++; | ||
} | ||
return thumbnails[thumbnails.length - 1].url; | ||
} | ||
|
||
/** | ||
* Truncates strings to 250 characters, attempting to preserve whole words | ||
* @param str {string} the string to truncate | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
import Formatters from 'static/js/formatters.js'; | ||
|
||
describe('image formatter', () => { | ||
const img = { | ||
url: 'https://a.mktgcdn.com/p/1024x768.jpg', | ||
width: 1024, | ||
height: 768, | ||
thumbnails: [ | ||
{ | ||
url: 'https://a.mktgcdn.com/p/619x348.jpg', | ||
width: 619, | ||
height: 348 | ||
}, | ||
{ | ||
url: 'https://a.mktgcdn.com/p/600x337.jpg', | ||
width: 600, | ||
height: 337 | ||
}, | ||
{ | ||
url: 'https://a.mktgcdn.com/p/196x110.jpg', | ||
width: 196, | ||
height: 110 | ||
} | ||
] | ||
} | ||
|
||
describe('when choosing the smallest image over threshold', () => { | ||
it('By default chooses the smallest image with width >= 200', () => { | ||
const imageUrl = Formatters.image(img).url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/600x337.jpg'); | ||
}); | ||
|
||
it('Can restrict the dimensions by width', () => { | ||
const imageUrl = Formatters.image(img, '601x').url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/619x348.jpg'); | ||
}); | ||
|
||
it('Can restrict by width when both dimensions specified', () => { | ||
const imageUrl = Formatters.image(img, '601x1').url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/619x348.jpg'); | ||
}); | ||
|
||
it('Can restrict the dimensions by height', () => { | ||
const imageUrl = Formatters.image(img, 'x338').url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/619x348.jpg'); | ||
}); | ||
|
||
it('Can restrict by height when both dimensions specified', () => { | ||
const imageUrl = Formatters.image(img, '1x338').url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/619x348.jpg'); | ||
}); | ||
|
||
it('Can restrict by width when both dimensions specified', () => { | ||
const imageUrl = Formatters.image(img, '601x0').url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/619x348.jpg'); | ||
}); | ||
|
||
it('return the largest image when no image is over threshold', () => { | ||
const imageUrl = Formatters.image(img, '99999x99999').url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/1024x768.jpg'); | ||
}); | ||
|
||
it('returns the smallest image when no dimensions given', () => { | ||
const imageUrl = Formatters.image(img, 'x').url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/196x110.jpg'); | ||
}); | ||
}); | ||
|
||
describe('when choosing the biggest image under threshold', () => { | ||
it('Can restrict the dimensions by width', () => { | ||
const imageUrl = Formatters.image(img, '601x', false).url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/600x337.jpg'); | ||
}); | ||
|
||
it('Can restrict by width when both dimensions specified', () => { | ||
const imageUrl = Formatters.image(img, '601x9999', false).url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/600x337.jpg'); | ||
}); | ||
|
||
it('Can restrict the dimensions by height', () => { | ||
const imageUrl = Formatters.image(img, 'x338', false).url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/600x337.jpg'); | ||
}); | ||
|
||
it('Can restrict by height when both dimensions specified', () => { | ||
const imageUrl = Formatters.image(img, '9999x338', false).url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/600x337.jpg'); | ||
}); | ||
|
||
it('returns the smallest image when no image is under threshold', () => { | ||
const imageUrl = Formatters.image(img, '-1x-1', false).url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/196x110.jpg'); | ||
}); | ||
|
||
it('return the largest image when no dimensions given', () => { | ||
const imageUrl = Formatters.image(img, 'x', false).url; | ||
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/1024x768.jpg'); | ||
}); | ||
}); | ||
}); |
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.
Do we need the
unshift
above if we're sorting here? I saw something about potential side effects if we alterimage.thumbnails
from how it was before. Do we need to worry about that?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.
There shouldn't be any side effects since we're now guaranteeing things are sorted the way we want, and I thought it might make things a tiny bit faster, since it looked like live api already sorts things, but I can remove it.
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.
If there aren't any adverse effects, no worries! I just saw you and Christian chatted about it before.