uniform product image size #941

yen-tt · 2021-09-08T12:56:13Z

update css for product-prominentimage-clickable and product-prominentimage cards to have fixed height of 300px that fill the whole width (maintain its aspect ratio, clipped to fit)
the height is a customizable css variable
since object-fit doesn't work in ie11, a polyfill library (object-fit-images) is used to adjust the image.

J=SLAP-1553
TEST=manual

add product entities with different image sizes, launched test site and see that all images are properly sized. tested in IE11, chome, and safari

- update css for product-prominentimage-clickable and product-prominentimage cards to have fixed height of 300px that fill the whole width (maintain its aspect ratio, clipped to fit) J=SLAP-1553 TEST=manual - add product entities with different image sizes, launched test site and see that all images are properly sized.

coveralls · 2021-09-08T12:57:52Z

Coverage increased (+0.1%) to 8.206% when pulling e62c166 on dev/image-card-styling into df97c5d on develop.

oshi97 · 2021-09-08T14:26:45Z

static/scss/answers/cards/product-prominentimage.scss

    width: 100%;
    flex-shrink: 0;
  }

  &-img
  {
    width: 100%;
-    height: auto;
+    height: 300px;
+    object-fit: cover;


does this work on ie11? I had issues getting object-fit to work on it before

you are right, it didn't work. I updated to use a div with background image instead of img tag, so we can use background-position and background-size similiar to object-position and object-fit. this should work in all browser

oshi97 · 2021-09-08T21:21:28Z

static/scss/answers/cards/product-prominentimage-clickable.scss

    width: 100%;
    flex-shrink: 0;
  }

  &-img
  {
    width: 100%;
-    height: auto;
+    height: 300px;


just checking that there's no drawbacks to having a fixed height that Rose might care about?

For uniform size, I think a fixed height have to be implemented. This was used in the HH link from the jira item as well. But it seems like Rose is reconsidering this change and leaving the images as what we have before.

oshi97

if Rose doesn't double back this lgtm!

oshi97 · 2021-09-09T15:47:13Z

cards/product-prominentimage/component.js

@@ -20,6 +20,10 @@ class product_prominentimageCardComponent extends BaseCard['product-prominentima
    }
    const linkTarget = AnswersExperience.runtimeConfig.get('linkTarget') || '_top';

+    //polyfill styling for ie11
+    var someImages = document.querySelectorAll('.HitchhikerProductProminentImage-img');
+    HitchhikerJS.objectFitImages(someImages);


would it be possible to do the global polyfill for this, and take this out of the specific card components? We probably don't want to run this every render, and this isn't really the responsibility of the card anyway. Maybe we could do it only the legacy bundle

I will try, does ie11 only use the legacy bundles?

So this can't be in entry-legacy.js because this script is invoked before the images on html page are loaded. The polyfill function require the src link populated in the img tag then convert to some inline css styling in the tag.

Ah ok got it, maybe we can shuffle this off to the base card component to hide some implementation? like maybe we can add a flag to the child card's constructor (or even have a list of cards in the base card component?)
taking it out of the child cards, in whatever way we decide, will also make it easier to tweak forked versions of these cards in the future.

Does this polyfill work in a way where it doesn't do anything if you run it on a browser that does support object-fit? If so I don't mind running this a few extra times on ie11, in order to improve our non ie-11 cases and keep the cards a little cleaner. If not, we could even have separate versions of this method in HitchhikerJS, depending on whether you are on the regular of legacy bundle.

the polyfill function would check if the element.style contains object-fit. If the browser tag supports it, it wouldn't do anything

I moved the implementations into the base card component, which check the list of cards that would require this polyfill. Let me know if that's fine or need tweaking!

oshi97 · 2021-09-09T20:31:41Z

cards/card_component.js

@@ -16,6 +16,12 @@ BaseCard["{{componentName}}"] = class extends ANSWERS.Component {
   * only if such a toggle is present in the DOM.
   */
  onMount() {
+    //polyfill for image styling (object-fit) in ie11
+    if (['product-prominentimage', 'product-prominentimage-clickable'].includes('{{componentName}}')) {


is this going to be an issue if somebody forks the card with a custom name? could we just run the polyfill on the entire card all the time?

you are right. updated!

oshi97 · 2021-09-09T20:38:56Z

cards/card_component.js

@@ -16,6 +16,10 @@ BaseCard["{{componentName}}"] = class extends ANSWERS.Component {
   * only if such a toggle is present in the DOM.
   */
  onMount() {
+    //polyfill for image styling (object-fit) in ie11
+    var images = document.querySelectorAll('.HitchhikerProductProminentImage-img');


I hate to be that guy but can we use const

ah nah my bad, missed this one

oshi97 · 2021-09-09T20:39:53Z

cards/card_component.js

@@ -16,6 +16,10 @@ BaseCard["{{componentName}}"] = class extends ANSWERS.Component {
   * only if such a toggle is present in the DOM.
   */
  onMount() {
+    //polyfill for image styling (object-fit) in ie11


I was thinking it might be worth it to run it on the entire card instead of just this css class, on the off chance somebody renames their card css classes, but this is probably fine for now!

oshi97

lgtm after changing to const!

## Version 1.24.0 ### Features - A new formatter was added that prints the list of categories applied to an entity. (#946) - Added a new localized price range formatter. (#943) ### Enhancements - Google's text highlighting was added to the "View Details" link on the `document-search` Direct Answer card. Note that the highlighting is only applied when the user's browser is Chrome. (#945) - The Open Status translations are no longer hard-coded in `static` JS files. Instead, they are sourced from the Theme's .PO files. This means they can be overridden in custom .PO files. ### Bugfixes - Styling was added to the `prominent-image` and `prominent-video` cards to ensure that images and videos, respectively, are the same size. (#941, #950)

oshi97 reviewed Sep 8, 2021

View reviewed changes

replace object-fit

d7c05ff

yen-tt requested a review from oshi97 September 8, 2021 20:48

oshi97 reviewed Sep 8, 2021

View reviewed changes

oshi97 approved these changes Sep 9, 2021

View reviewed changes

Yen Truong added 2 commits September 9, 2021 11:15

remove changes to template, use pollyfill

25a0669

made height into a variable

d7e9fd9

yen-tt requested a review from oshi97 September 9, 2021 15:45

oshi97 reviewed Sep 9, 2021

View reviewed changes

yen-tt requested review from oshi97, tmeyer2115 and nmanu1 September 9, 2021 19:15

move polyfill to base card component

4afb282

oshi97 reviewed Sep 9, 2021

View reviewed changes

run polyfil on base card

cfc0e65

oshi97 reviewed Sep 9, 2021

View reviewed changes

oshi97 approved these changes Sep 9, 2021

View reviewed changes

change to const

e62c166

yen-tt merged commit e9ffd0c into develop Sep 9, 2021

yen-tt deleted the dev/image-card-styling branch September 9, 2021 20:50

tmeyer2115 mentioned this pull request Sep 14, 2021

v1.24 #951

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uniform product image size #941

uniform product image size #941

yen-tt commented Sep 8, 2021 •

edited

Loading

coveralls commented Sep 8, 2021 •

edited

Loading

oshi97 Sep 8, 2021

yen-tt Sep 8, 2021

oshi97 Sep 8, 2021

yen-tt Sep 8, 2021

oshi97 left a comment

oshi97 Sep 9, 2021

yen-tt Sep 9, 2021

oshi97 Sep 9, 2021

yen-tt Sep 9, 2021 •

edited

Loading

oshi97 Sep 9, 2021

oshi97 Sep 9, 2021 •

edited

Loading

yen-tt Sep 9, 2021 •

edited

Loading

yen-tt Sep 9, 2021 •

edited

Loading

oshi97 Sep 9, 2021

yen-tt Sep 9, 2021

oshi97 Sep 9, 2021 •

edited

Loading

yen-tt Sep 9, 2021

oshi97 Sep 9, 2021

oshi97 left a comment

uniform product image size #941

uniform product image size #941

Conversation

yen-tt commented Sep 8, 2021 • edited Loading

coveralls commented Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshi97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yen-tt Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshi97 Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

yen-tt Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

yen-tt Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshi97 Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshi97 left a comment

Choose a reason for hiding this comment

yen-tt commented Sep 8, 2021 •

edited

Loading

coveralls commented Sep 8, 2021 •

edited

Loading

yen-tt Sep 9, 2021 •

edited

Loading

oshi97 Sep 9, 2021 •

edited

Loading

yen-tt Sep 9, 2021 •

edited

Loading

yen-tt Sep 9, 2021 •

edited

Loading

oshi97 Sep 9, 2021 •

edited

Loading