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 3 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
43 changes: 19 additions & 24 deletions static/js/formatters-internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,9 @@ export function image(simpleOrComplexImage = {}, size = '200x', atLeastAsLarge =
});
}

let height, index;
let width = (height = -1);
let index;
let height = -1;
let width = -1;
let desiredDims = desiredSize.split('x');

if (desiredDims[0] !== '') {
Expand All @@ -351,29 +352,23 @@ export function image(simpleOrComplexImage = {}, size = '200x', atLeastAsLarge =

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;
}
const smallestValidThumbnail = image.thumbnails
.reduce((storedThumbnail, currentThumbnail) => {
if (!currentThumbnail.width || !currentThumbnail.height) {
return storedThumbnail;
}
widthOk = width > 0 ? (currentThumbnail.width >= width) : widthOk;
heightOk = height > 0 ? (currentThumbnail.height >= height) : heightOk;
Copy link
Member

Choose a reason for hiding this comment

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

What are your thoughts on this renaming width and height to minWidth and minHeight? Also, maybe we could use an if statement like if (desiredDims) to get rid of the ternary operators

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the other half of the code (when atLeastAsLarge is false) has width and height as maxWidth and maxHeight, and tries the pick the biggest image that is smaller than that. I'll do a bigger refactor since I don't think that code is working as expected either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed a larger refactor!

if (!heightOk || !widthOk) {
return storedThumbnail
}
if (currentThumbnail.width < storedThumbnail.width) {
tmeyer2115 marked this conversation as resolved.
Show resolved Hide resolved
return currentThumbnail;
}
return storedThumbnail;
}, image.thumbnails[0]);
return smallestValidThumbnail.url;
} else {
index = 0;

Expand Down
40 changes: 40 additions & 0 deletions tests/static/js/formatters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,44 @@ describe('Formatters', () => {
expect(distance).toEqual('10,000.0 km');
});
})

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
}
]
}

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 the dimensions by height', () => {
const imageUrl = Formatters.image(img, 'x338').url;
expect(imageUrl).toEqual('https://a.mktgcdn.com/p/619x348.jpg');
});
});
});