From 0a407709c8a5553c2f2cb27593267e69f628dbe9 Mon Sep 17 00:00:00 2001 From: nmanu1 Date: Mon, 8 Nov 2021 14:17:20 -0500 Subject: [PATCH 1/5] Update image formatter --- static/js/formatters-internal.js | 101 +++++++++++-------------------- 1 file changed, 35 insertions(+), 66 deletions(-) diff --git a/static/js/formatters-internal.js b/static/js/formatters-internal.js index dbbe9bcb9..29cc0f3b3 100644 --- a/static/js/formatters-internal.js +++ b/static/js/formatters-internal.js @@ -294,7 +294,7 @@ export function image(simpleOrComplexImage = {}, size = '200x', atLeastAsLarge = return img; } - function imageBySizeEntity(image, desiredSize, atLeastAsLarge = true) { + function createDynamicUrl(image, desiredSize, atLeastAsLarge = true) { if ((image == null) || !(Object.prototype.toString.call(image).indexOf('Object') > 0)) { throw new Error("Expected parameter of type Map"); } @@ -308,22 +308,6 @@ export function image(simpleOrComplexImage = {}, size = '200x', atLeastAsLarge = throw new Error(`Object of type boolean expected. Got ${typeof atLeastAsLarge}.`); } - if (!image.thumbnails) { - image.thumbnails = []; - } - - if (!Array.isArray(image.thumbnails)) { - throw new Error(`Object of type array expected. Got ${typeof image.thumbnails}.`); - } - - if (image.width != undefined && image.height != undefined && image.url != undefined) { - image.thumbnails.push({ - 'width': image.width, - 'height': image.height, - 'url': image.url - }); - } - let desiredWidth, desiredHeight; let desiredDims = desiredSize.split('x'); @@ -340,15 +324,28 @@ export function image(simpleOrComplexImage = {}, size = '200x', atLeastAsLarge = throw new Error("Invalid height specified"); } } - 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 [urlWithoutExtension, extension] = _splitUrlOnIndex(image.url, image.url.lastIndexOf('.')); + const [urlBeforeDimensions, dimensions] = _splitUrlOnIndex(urlWithoutExtension, urlWithoutExtension.lastIndexOf('/') + 1); + + if (desiredDims[0] === '' || desiredDims[1] === '') { + if (atLeastAsLarge) { + desiredWidth = desiredWidth ?? 1; + desiredHeight = desiredHeight ?? 1; + } else { + const fullSizeDims = dimensions.split('x'); + desiredWidth = desiredWidth ?? Number.parseInt(fullSizeDims[0]); + desiredHeight = desiredHeight ?? Number.parseInt(fullSizeDims[1]); + } + } + + const urlWithDesiredDims = urlBeforeDimensions + desiredWidth + 'x' + desiredHeight + extension; + + return atLeastAsLarge ? _replaceUrlHost(urlWithDesiredDims, 'dynl.mktgcdn.com') + : _replaceUrlHost(urlWithDesiredDims, 'dynm.mktgcdn.com'); } - const result = imageBySizeEntity(img, size, atLeastAsLarge); + const result = createDynamicUrl(img, size, atLeastAsLarge); return Object.assign( {}, @@ -360,55 +357,27 @@ 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} + * Splits a url into two parts at the specified index. + * + * @param {string} url + * @param {number} index + * @returns {Array} */ -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; +function _splitUrlOnIndex(url, index) { + return [url.slice(0, index), url.slice(index)]; } /** - * 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. + * Replaces the current host of a url with the specified host. * - * 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 + * @param {string} url + * @param {string} host * @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; +function _replaceUrlHost(url, host) { + const splitUrl = url.split('://'); + const urlAfterHost = splitUrl[1].slice(splitUrl[1].indexOf('/')); + return splitUrl[0] + '://' + host + urlAfterHost; } /** From 71caf09495dfcdbcad2554155765c02bbabbb6a5 Mon Sep 17 00:00:00 2001 From: nmanu1 Date: Mon, 8 Nov 2021 14:46:02 -0500 Subject: [PATCH 2/5] Update tests --- tests/static/js/formatters-internal/image.js | 72 ++++---------------- 1 file changed, 14 insertions(+), 58 deletions(-) diff --git a/tests/static/js/formatters-internal/image.js b/tests/static/js/formatters-internal/image.js index 01996bbb1..c714334d9 100644 --- a/tests/static/js/formatters-internal/image.js +++ b/tests/static/js/formatters-internal/image.js @@ -2,99 +2,55 @@ 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 - } - ] + url: 'https://a.mktgcdn.com/p/1024x768.jpg' } 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'); + expect(imageUrl).toEqual('https://dynl.mktgcdn.com/p/200x1.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'); + expect(imageUrl).toEqual('https://dynl.mktgcdn.com/p/601x1.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'); + expect(imageUrl).toEqual('https://dynl.mktgcdn.com/p/1x338.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('Can restrict by both dimensions', () => { + const imageUrl = Formatters.image(img, '601x338').url; + expect(imageUrl).toEqual('https://dynl.mktgcdn.com/p/601x338.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'); + expect(imageUrl).toEqual('https://dynl.mktgcdn.com/p/1x1.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'); + expect(imageUrl).toEqual('https://dynm.mktgcdn.com/p/601x768.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'); + expect(imageUrl).toEqual('https://dynm.mktgcdn.com/p/1024x338.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('Can restrict by both dimensions', () => { + const imageUrl = Formatters.image(img, '999x338', false).url; + expect(imageUrl).toEqual('https://dynm.mktgcdn.com/p/999x338.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'); + expect(imageUrl).toEqual('https://dynm.mktgcdn.com/p/1024x768.jpg'); }); }); }); \ No newline at end of file From 069374cebe32d263a9a97299228e1180e81c2ebf Mon Sep 17 00:00:00 2001 From: nmanu1 Date: Mon, 8 Nov 2021 16:09:08 -0500 Subject: [PATCH 3/5] Feedback --- static/js/formatters-internal.js | 114 +++++++++++++++---------------- 1 file changed, 54 insertions(+), 60 deletions(-) diff --git a/static/js/formatters-internal.js b/static/js/formatters-internal.js index 29cc0f3b3..cbeb541e9 100644 --- a/static/js/formatters-internal.js +++ b/static/js/formatters-internal.js @@ -281,77 +281,71 @@ export function joinList(list, separator) { return list.join(separator); } -/* - * Given object with url and alternateText, changes url to use https +/** + * Given an image object with a url, changes the url to use dynamic thumbnailer and https. + * + * @param {Object} simpleOrComplexImage An image object with a url + * @param {string} desiredSize The desired size of the image ('x') + * @param {boolean} atLeastAsLarge Whether the image should be larger than the desired size or smaller + * @returns {Object} An object with a url for dynamic thumbnailer */ -export function image(simpleOrComplexImage = {}, size = '200x', atLeastAsLarge = true) { - let img = simpleOrComplexImage.image || simpleOrComplexImage; - if (!img) { +export function image(simpleOrComplexImage = {}, desiredSize = '200x', atLeastAsLarge = true) { + let image = simpleOrComplexImage.image || simpleOrComplexImage; + if (!image) { return {}; } - - if (!img.url) { - return img; + if (!image.url) { + return image; + } + if (!(Object.prototype.toString.call(image).indexOf('Object') > 0)) { + throw new Error("Expected parameter of type Map"); + } + if ((typeof desiredSize !== 'string') || (desiredSize == null)) { + throw new Error(`Object of type string expected. Got ${typeof desiredSize}.`); + } + if (desiredSize.indexOf('x') === -1) { + throw new Error("Invalid desired size"); + } + if ((typeof atLeastAsLarge !== 'boolean') || (atLeastAsLarge == null)) { + throw new Error(`Object of type boolean expected. Got ${typeof atLeastAsLarge}.`); } - function createDynamicUrl(image, desiredSize, atLeastAsLarge = true) { - if ((image == null) || !(Object.prototype.toString.call(image).indexOf('Object') > 0)) { - throw new Error("Expected parameter of type Map"); - } - if ((typeof desiredSize !== 'string') || (desiredSize == null)) { - throw new Error(`Object of type string expected. Got ${typeof desiredSize}.`); - } - if (desiredSize.indexOf('x') === -1) { - throw new Error("Invalid desired size"); - } - if ((typeof atLeastAsLarge !== 'boolean') || (atLeastAsLarge == null)) { - throw new Error(`Object of type boolean expected. Got ${typeof atLeastAsLarge}.`); - } - - let desiredWidth, desiredHeight; - let desiredDims = desiredSize.split('x'); + let desiredWidth, desiredHeight; + let desiredDims = desiredSize.split('x'); - if (desiredDims[0] !== '') { - desiredWidth = Number.parseInt(desiredDims[0]); - if (Number.isNaN(desiredWidth)) { - throw new Error("Invalid width specified"); - } - } + const [urlWithoutExtension, extension] = _splitUrlOnIndex(image.url, image.url.lastIndexOf('.')); + const [urlBeforeDimensions, dimensions] = _splitUrlOnIndex(urlWithoutExtension, urlWithoutExtension.lastIndexOf('/') + 1); + const fullSizeDims = dimensions.split('x'); - if (desiredDims[1] !== '') { - desiredHeight = Number.parseInt(desiredDims[1]); - if (Number.isNaN(desiredHeight)) { - throw new Error("Invalid height specified"); - } + if (desiredDims[0] !== '') { + desiredWidth = Number.parseInt(desiredDims[0]); + if (Number.isNaN(desiredWidth)) { + throw new Error("Invalid width specified"); } + } else { + desiredWidth = atLeastAsLarge ? 1 : Number.parseInt(fullSizeDims[0]); + } - const [urlWithoutExtension, extension] = _splitUrlOnIndex(image.url, image.url.lastIndexOf('.')); - const [urlBeforeDimensions, dimensions] = _splitUrlOnIndex(urlWithoutExtension, urlWithoutExtension.lastIndexOf('/') + 1); - - if (desiredDims[0] === '' || desiredDims[1] === '') { - if (atLeastAsLarge) { - desiredWidth = desiredWidth ?? 1; - desiredHeight = desiredHeight ?? 1; - } else { - const fullSizeDims = dimensions.split('x'); - desiredWidth = desiredWidth ?? Number.parseInt(fullSizeDims[0]); - desiredHeight = desiredHeight ?? Number.parseInt(fullSizeDims[1]); - } + if (desiredDims[1] !== '') { + desiredHeight = Number.parseInt(desiredDims[1]); + if (Number.isNaN(desiredHeight)) { + throw new Error("Invalid height specified"); } + } else { + desiredHeight = atLeastAsLarge ? 1 : Number.parseInt(fullSizeDims[1]); + } - const urlWithDesiredDims = urlBeforeDimensions + desiredWidth + 'x' + desiredHeight + extension; + const urlWithDesiredDims = urlBeforeDimensions + desiredWidth + 'x' + desiredHeight + extension; - return atLeastAsLarge ? _replaceUrlHost(urlWithDesiredDims, 'dynl.mktgcdn.com') + const dynamicUrl = atLeastAsLarge + ? _replaceUrlHost(urlWithDesiredDims, 'dynl.mktgcdn.com') : _replaceUrlHost(urlWithDesiredDims, 'dynm.mktgcdn.com'); - } - - const result = createDynamicUrl(img, size, atLeastAsLarge); return Object.assign( {}, - img, + image, { - url: result.replace('http://', 'https://') + url: dynamicUrl.replace('http://', 'https://') } ); } @@ -359,9 +353,9 @@ export function image(simpleOrComplexImage = {}, size = '200x', atLeastAsLarge = /** * Splits a url into two parts at the specified index. * - * @param {string} url - * @param {number} index - * @returns {Array} + * @param {string} url The url to be split + * @param {number} index The index at which to split the url + * @returns {Array} The two parts of the url after splitting */ function _splitUrlOnIndex(url, index) { return [url.slice(0, index), url.slice(index)]; @@ -370,9 +364,9 @@ function _splitUrlOnIndex(url, index) { /** * Replaces the current host of a url with the specified host. * - * @param {string} url - * @param {string} host - * @returns {string} + * @param {string} url The url whose host is to be changed + * @param {string} host The new host to change to + * @returns {string} The url updated with the specified host */ function _replaceUrlHost(url, host) { const splitUrl = url.split('://'); From 06e1d9cb87b93af35426cbdaa2bc0a8bed9740c0 Mon Sep 17 00:00:00 2001 From: nmanu1 Date: Mon, 8 Nov 2021 18:39:43 -0500 Subject: [PATCH 4/5] Rename --- static/js/formatters-internal.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/static/js/formatters-internal.js b/static/js/formatters-internal.js index cbeb541e9..7a8af205e 100644 --- a/static/js/formatters-internal.js +++ b/static/js/formatters-internal.js @@ -313,8 +313,8 @@ export function image(simpleOrComplexImage = {}, desiredSize = '200x', atLeastAs let desiredWidth, desiredHeight; let desiredDims = desiredSize.split('x'); - const [urlWithoutExtension, extension] = _splitUrlOnIndex(image.url, image.url.lastIndexOf('.')); - const [urlBeforeDimensions, dimensions] = _splitUrlOnIndex(urlWithoutExtension, urlWithoutExtension.lastIndexOf('/') + 1); + const [urlWithoutExtension, extension] = _splitStringOnIndex(image.url, image.url.lastIndexOf('.')); + const [urlBeforeDimensions, dimensions] = _splitStringOnIndex(urlWithoutExtension, urlWithoutExtension.lastIndexOf('/') + 1); const fullSizeDims = dimensions.split('x'); if (desiredDims[0] !== '') { @@ -351,14 +351,14 @@ export function image(simpleOrComplexImage = {}, desiredSize = '200x', atLeastAs } /** - * Splits a url into two parts at the specified index. + * Splits a string into two parts at the specified index. * - * @param {string} url The url to be split - * @param {number} index The index at which to split the url - * @returns {Array} The two parts of the url after splitting + * @param {string} str The string to be split + * @param {number} index The index at which to split the string + * @returns {Array} The two parts of the string after splitting */ -function _splitUrlOnIndex(url, index) { - return [url.slice(0, index), url.slice(index)]; +function _splitStringOnIndex(str, index) { + return [str.slice(0, index), str.slice(index)]; } /** From d180df83c0fc8e1b63201d1c93e53248b63a4e57 Mon Sep 17 00:00:00 2001 From: nmanu1 Date: Tue, 9 Nov 2021 11:55:00 -0500 Subject: [PATCH 5/5] Update jsdoc --- static/js/formatters-internal.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/static/js/formatters-internal.js b/static/js/formatters-internal.js index 7a8af205e..9682c8e2f 100644 --- a/static/js/formatters-internal.js +++ b/static/js/formatters-internal.js @@ -284,9 +284,19 @@ export function joinList(list, separator) { /** * Given an image object with a url, changes the url to use dynamic thumbnailer and https. * + * Note: A dynamic thumbnailer url generated with atLeastAsLarge = true returns an image that is + * at least as large in one dimension of the desired size. In other words, the returned image will + * be at least as large, and as close as possible to, the largest image that is contained within a + * box of the desired size dimensions. + * + * If atLeastAsLarge = false, the dynamic thumbnailer url will give the largest image that is + * smaller than the desired size in both dimensions. + * * @param {Object} simpleOrComplexImage An image object with a url * @param {string} desiredSize The desired size of the image ('x') - * @param {boolean} atLeastAsLarge Whether the image should be larger than the desired size or smaller + * @param {boolean} atLeastAsLarge Whether the image should be at least as large as the desired + * size in one dimension or smaller than the desired size in both + * dimensions. * @returns {Object} An object with a url for dynamic thumbnailer */ export function image(simpleOrComplexImage = {}, desiredSize = '200x', atLeastAsLarge = true) {