-
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
Conversation
Prior to this commit, the image formatter would always choose the last thumbnail in the image.thumbnails array. @creotutar said that the desired behavior is to pick the smallest one that is at least as big as the `desiredSize` parameter. It looks like the image formatter is assuming that the thumbnails are already sorted largest to smallest, which is not necessarily the case. TEST=manual,auto tested that yanswers now chooses the correct thumbnail added unit tests
static/js/formatters-internal.js
Outdated
widthOk = width > 0 ? (currentThumbnail.width >= width) : widthOk; | ||
heightOk = height > 0 ? (currentThumbnail.height >= height) : heightOk; |
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.
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
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.
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
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.
pushed a larger refactor!
static/js/formatters-internal.js
Outdated
*/ | ||
function _getSmallestThumbnailOverThreshold(thumbnails, minWidth, minHeight) { | ||
const thumbnailsOverThreshold = thumbnails.filter(thumb => { | ||
if (minWidth) { |
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 we're only working with minWidth
for now, I'd suggest getting rid of minHeight
here and maxHeight
below. I think this method will confuse someone who provides actual values for both. The filter
does not filter on both if both are present. It only filters on minWidth
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.
somebody is allowed to provide only minHeight if they want, by passing something like 'x200' which will have a minHeight of 200 and no minWidth
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.
discussed offline, I missed a case!
7b2ee92
to
95b0f39
Compare
const thumbnails = image.thumbnails.filter(thumb => thumb.width && thumb.height); | ||
const thumbnails = image.thumbnails | ||
.filter(thumb => thumb.width && thumb.height) | ||
.sort((a, b) => b.width - a.width); |
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 alter image.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.
: storedThumb | ||
}, thumbnailsOverThreshold[0]); | ||
return smallestValidThumb.url; | ||
return thumbnails[0].url; |
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.
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.
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.
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 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.
## Version 1.17.0 Version 1.17.0 of the Theme supports the new Overlay integration. It also has a number of changes to support the new, built-in Product entity type. ### Changes - A new file, static/js/formatters-custom.js was added to the Theme. This file is where any custom formatters or overrides of built-in formatters, should live. (#448) - Single language cards now use the `toLocalizedDistance` formatter to properly format entity distance. (#449) - To better support WCAG, the iFrame integration now has a `title` attribute on the iFrame. (#451) - The Theme now allows a new type of Answers integration: the Overlay. This is one of the simplest integrations offered. Additional documentation on it can be found in the Hitchhiker training. - We upgraded the Theme to use Webpack v5. This will enable proper tree-shaking of bundle.js. (#463, #470) - A new card type was added: `product-prominentimage-clickable`. The card itself is clickable. (#466) - A new formatter was added: `Formatter.hoursList`. This formatter lists the hours for an entity for every day of the week. (#467) - To improve site performance, the `defer` attribute was added to all the SDK JS assets. (#473, #474) - Small updates were made to all the product cards to support the new, built-in Product entity type. (#411, #476) - A new formatter was added for prices: `Formatter.price`. (#472) - The `JAMBO_INJECTED_DATA` environment variable now includes the label of an experience's vertical. This label is used as the default vertical name in navigation, no results, and universal section title. (#490, 493) ### Bug Fixes - The post upgrade script now works correctly when the Theme has been imported as a submodule. (#441) - The `LocationStandard` card's styling was updated to be fully compatible with IE11. (#455) - A bug was fixed that caused the Theme's inline Javascript to be minified twice. By removing this bug, Webpack builds were made considerably more performant. (#461) - Fixes were made to ensure the `View Results` button for `CollapsibleFilters` is sticky. (#468, #469) - The logic for identifying absolute URLs in the HBS templates was corrected (#475) - The thumbnail selection logic in `Formatter.image` was incorrect. It was not properly selecting the smallest thumbnail greater than the provided threshold. This has been corrected. Note that this bug had an impact on page performance as unnecessarily large images were loaded on the page. (#471) - The `relativePath` computed by Jambo is now properly passed along to the Theme's cards. (#499)
## Version 1.17.0 Version 1.17.0 of the Theme supports the new Overlay integration. It also has a number of changes to support the new, built-in Product entity type. ### Changes - A new file, static/js/formatters-custom.js was added to the Theme. This file is where any custom formatters or overrides of built-in formatters, should live. (#448) - Single language cards now use the `toLocalizedDistance` formatter to properly format entity distance. (#449) - To better support WCAG, the iFrame integration now has a `title` attribute on the iFrame. (#451) - The Theme now allows a new type of Answers integration: the Overlay. This is one of the simplest integrations offered. Additional documentation on it can be found in the Hitchhiker training. - We upgraded the Theme to use Webpack v5. This will enable proper tree-shaking of bundle.js. (#463, #470) - A new card type was added: `product-prominentimage-clickable`. The card itself is clickable. (#466) - A new formatter was added: `Formatter.hoursList`. This formatter lists the hours for an entity for every day of the week. (#467) - To improve site performance, the `defer` attribute was added to all the SDK JS assets. (#473, #474) - Small updates were made to all the product cards to support the new, built-in Product entity type. (#411, #476) - A new formatter was added for prices: `Formatter.price`. (#472) - The `JAMBO_INJECTED_DATA` environment variable now includes the label of an experience's vertical. This label is used as the default vertical name in navigation, no results, and universal section title. (#490, 493) ### Bug Fixes - The post upgrade script now works correctly when the Theme has been imported as a submodule. (#441) - The `LocationStandard` card's styling was updated to be fully compatible with IE11. (#455) - A bug was fixed that caused the Theme's inline Javascript to be minified twice. By removing this bug, Webpack builds were made considerably more performant. (#461) - Fixes were made to ensure the `View Results` button for `CollapsibleFilters` is sticky. (#468, #469) - The logic for identifying absolute URLs in the HBS templates was corrected (#475) - The thumbnail selection logic in `Formatter.image` was incorrect. It was not properly selecting the smallest thumbnail greater than the provided threshold. This has been corrected. Note that this bug had an impact on page performance as unnecessarily large images were loaded on the page. (#471) - The `relativePath` computed by Jambo is now properly passed along to the Theme's cards. (#499)
Prior to this commit, the image formatter would always choose the last
thumbnail in the image.thumbnails array.
@creotutar said that the desired behavior is to pick
the smallest one that is at least as big as the desiredSize
parameter.
It looks like the image formatter is assuming that the thumbnails
are already sorted largest to smallest, which I'm not sure is always the case.
If this is the case, an alternative fix could be to do an unshift instead of a push
here
answers-hitchhiker-theme/static/js/formatters-internal.js
Line 327 in 49ec003
TEST=manual,auto
tested that yanswers now chooses the correct thumbnail (600x337 instead of 1044x588)
added unit tests