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

Jump to bottom

image formatter: choose the smallest thumbnail that is big enough #471

Merged
merged 15 commits into from
Nov 16, 2020
Merged

image formatter: choose the smallest thumbnail that is big enough #471

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
da061f1
image formatter: choose the smallest thumbnail that is big enough
oshi97 Nov 12, 2020
e6a45ef
just do an unshift instead?
oshi97 Nov 12, 2020
49ec003
revert use unshift
oshi97 Nov 13, 2020
dabf325
bigger refactor
oshi97 Nov 16, 2020
2139a7b
add test
oshi97 Nov 16, 2020
971fd33
add back newline
oshi97 Nov 16, 2020
0bed343
update names, add jsdoc
oshi97 Nov 16, 2020
3b8f175
names
oshi97 Nov 16, 2020
dc7835d
remove extra newline
oshi97 Nov 16, 2020
97d59ab
fix bug tom pointed out
oshi97 Nov 16, 2020
1e09783
Try to match what the old behavior likely intended
oshi97 Nov 16, 2020
e90a57d
fix typo
oshi97 Nov 16, 2020
95b0f39
update jsdoc
oshi97 Nov 16, 2020
00a5011
clearer jest descriptions
oshi97 Nov 16, 2020
0a9cd10
remove ifPossible
oshi97 Nov 16, 2020
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
125 changes: 64 additions & 61 deletions static/js/formatters-internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,84 +324,35 @@ export function image(simpleOrComplexImage = {}, size = '200x', atLeastAsLarge =
}

if (image.width != undefined && image.height != undefined && image.url != undefined) {
image.thumbnails.push({
image.thumbnails.unshift({
oshi97 marked this conversation as resolved.
Show resolved Hide resolved
'width': image.width,
'height': image.height,
'url': image.url
});
}

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);
Copy link
Collaborator

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 alter image.thumbnails from how it was before. Do we need to worry about that?

Copy link
Contributor Author

@oshi97 oshi97 Nov 16, 2020 •
edited
Loading

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.

Copy link
Collaborator

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.

return atLeastAsLarge
? _getSmallestThumbnailOverThresholdIfPossible(thumbnails, desiredWidth, desiredHeight)
: _getLargestThumbnailUnderThresholdIfPossible(thumbnails, desiredWidth, desiredHeight);
}

const result = imageBySizeEntity(img, size, atLeastAsLarge);
Expand All @@ -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 _getSmallestThumbnailOverThresholdIfPossible(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;
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 _getLargestThumbnailUnderThresholdIfPossible. The IfPossible suffix is throwing me off too. That name hints to me that the method may return null or undefined if not possible. But, this returns a sane default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 _getLargestThumbnailUnderThresholdIfPossible(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
Expand Down
100 changes: 100 additions & 0 deletions tests/static/js/formatters-internal/image.js
View file
Open in desktop
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 at least as large as', () => {
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 fits the dimensions', () => {
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 at most as large as', () => {
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 fits the dimensions', () => {
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');
});
});
});