Fix collage section product and collection card image size

[KaichenWang]

Why are these changes introduced?

Fixes #995

Also addresses:

#929 (comment)
#378
#699
#910

What approach did you take?

1. Prevent "squished" images

Product and collection card images:

• rendered in their native aspect ratio
• cannot be "squished" beyond minimum height (as determined by aspect ratio)
• can stretch vertically to fill up remaining vertical space (see point # 2)

This is a change in how the collage inherently behaves (see before and after demo stores), but will prevent narrow horizontally cropped product/collection images.

2. Extend card height

• Allow product and collection cards to extend height to fill available vertical container space
• Boolean extend_height can be passed into card-product and card-collection snippets

3. Refactor - Separate collage grid layout from block content

This allows styles for collage grid layout to be decoupled from each block's contents. This simplifies the CSS structure.

Layout

element	purpose
.collage	container grid
.collage__item	grid items; containers for block content

Block content

block type	in Liquid
image	.collage-card .media
product	card-product snippet
collection	card-collection snippet
video	.collage-card .deferred-media

4. Refactor - Separate video modal into its own component

• Video modal could be used outside of a collage context, so makes sense to decouple from collage.
• Simplify BEM naming convention

before	after
in collage.css	in component-modal-video.css
.collage-video__modal	.modal-video
.collage-video__modal-<element>	.modal-video__<element>

Other considerations

Simplification of collage video behaviour

Before

• If video is the only collage block, play video inline
• If video is not the only collage block, play video in modal

After

• Always play collage video in modal

Why?

• Was an instance of "magic", where it may be unclear to a merchant why the collage video has 2 different behaviours
• Removes significant repeated code in Liquid
• Thanks to vertical spacing settings, the existing Video section can now essentially be used in conjunction with a collage in order to achieve inline video playback

Demo links

Store

Editor

⚠️ For comparison only - Dawn 2.4.0 release from theme store: Store, Editor

Checklist

• Followed theme code principles
• Linted with Theme Check
• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility

[ludoboludo]

I only reviewed some of the functionality and have a few things I noted.

• The focused image used to dictate the overall height of the section and wouldn't crop the image/media in it unless it's very small. Seems like now the height is quite tall by default, I'd say too tall in general. If we're wanting to remove that previous functionality then it would probably make sense to add a section height setting like some other sections. We could have an adapt to focused block image as well 🤷 screenshot.
  This change would have an impact on how people used to use the section but I guess it would be part of a breaking change so it's not like we would update people to this version without their consent.

• A color scheme could be nice on the section level but I see Meli mentioned that it would be done after future explorations.

• The collection block title seem a little to big when there isn't an image in comparison to the section title and product block title. I had started a PR to fix it but might as well be done in this overall rework of the section. screenshot

• The spacing on the product card that has no image seem a little too far away from the title with the border: screenshot. Maybe the space above it could reduced to visually convey that they're related.

• In the context of this section, should the image and video block be considered "cards" ? I just find it odd when you're tweaking the cards' border and shadow, that it impacts only some blocks. So if you want the overall look of the section to be similar you have to tweak different stuff in the theme settings. I guess with potential section overrides it would make it work more easily. screenshot

• On mobile it looks like if you have two blocks the layout isn't what it should be: screenshot.

• There is this situation with collage where if you remove the title of the second collage section (when you have two of them in a row), it then looks like one section. So I think in this situation we should use the multicolumn vertical space instead of the vertical space between sections 🤔 screenshot

• On the video block, the shadow setting doesn't not seem to be setup though the border is working: screenshot

[KaichenWang]

@ludoboludo

Thanks for reviewing!

I've addressed the following

• On mobile it looks like if you have two blocks the layout isn't what it should be: screenshot.

• On the video block, the shadow setting doesn't not seem to be setup though the border is working: screenshot

Here's some more info on the other points

• The focused image used to dictate the overall height of the section and wouldn't crop the image/media in it unless it's very small. Seems like now the height is quite tall by default, I'd say too tall in general. If we're wanting to remove that previous functionality then it would probably make sense to add a section height setting like some other sections. We could have an adapt to focused block image as well

Correct, the desktop grid item behaviour has changed

Before

After

All collage blocks' images try to maintain their native aspect ratio, but will expand to fit any available vertical space. The key difference is that images cannot shrink vertically anymore. This helps maintain overall larger surface area for images in the collage. Maximizing surface area for images is important, because global settings like border, border-radius, image padding and large grid gaps can start to eat into the space for the image itself.

• A color scheme could be nice on the section level but I see Meli mentioned that it would be done after future explorations.

Correct, this would be implemented as part of block-level overrides.

• The collection block title seem a little to big when there isn't an image in comparison to the section title and product block title. I had started a PR to fix it but might as well be done in this overall rework of the section. screenshot

The collection block now shares all markup and styles with the card-collection component. I agree the font-size can be reduced here, but would prefer to make that change separately; to ensure more visibility for the change and proper testing globally.

• The spacing on the product card that has no image seem a little too far away from the title with the border: screenshot. Maybe the space above it could reduced to visually convey that they're related.

Same for the product block, which now shares all markup and styles with the card-product component. Would prefer to test the proposed change in more scenarios and ship separately.

• In the context of this section, should the image and video block be considered "cards" ? I just find it odd when you're tweaking the cards' border and shadow, that it impacts only some blocks. So if you want the overall look of the section to be similar you have to tweak different stuff in the theme settings. I guess with potential section overrides it would make it work more easily. screenshot

Yes, separate issue was created for this

• There is this situation with collage where if you remove the title of the second collage section (when you have two of them in a row), it then looks like one section. So I think in this situation we should use the multicolumn vertical space instead of the vertical space between sections 🤔 screenshot

Yeah, although it could be beneficial for automatically enforcing consistent grid spacing, the decision was made to avoid "magic" behaviour. The section's internal padding settings should be leveraged to achieve the same look.

[melissaperreault]

Thank you Kai! I think I wasn't looking at the right demo link, sorry! 🙌

• Note FYI: The Image block would lose the color scheme once we Cards-ify all Collage blocks with [Global settings] Collage blocks to all take Cards styles #1015

[tyleralsbury]

Looks really good. I'm loving the simplification of the collage CSS. I think a lot of what we've done in the past few weeks has added some complexity to our styles in particular, but a PR like this helps to reel it in a bit. 🎉

I did some tests on various layouts and everything looked good to me. I will trust @wiktoriaswiecicka 's certainly more thorough visual tests.

[ludoboludo]

Looks good. I get now that change in logic. That we would rather crop the image from the image block or video one over a product or collection 👍

I think it has still an issue where if you're adding an image/video you will have a hard knowing what would be the right size to get the least amount of your content cropped.

Unless the styling has changed for mobile I noticed that it wasn't quite right when you have two blocks only.

[ludoboludo]

Should it span 2 when it's only two items ? Or be the two blocks on 1 row, 50% 50%.

[KaichenWang]

Yes good point, should be 50/50 if mobile layout is collage! Updated 44485c5