[Product layout] Add desktop media position

[andrewetchen]

PR Summary

This PR adds a desktop media position setting to control the layout on the product page and featured product section.

Why are these changes introduced?

Fixes #1690

What approach did you take?

My original approach was to use flex-direction: row-reverse to position the media to the right, similar to the Desktop image placement setting of the Image with text section. But this approach went against accessibility due to the amount of other elements within the media wrapper custom element. In order for this PDP enhancement to work correctly, we had to make sure that the DOM order wasn't "reversed" when using the right media position and we also needed to make sure the tabbing order made sense and didn't cause confusion.

The option we were left with was to add another media wrapper element to the main-product and featured-product sections. And although this adds duplicate code, it's a good solution that satisfies the above. Pending newer updates to browsers and/or updates to CSS, we can definitely revisit this and improve it. The additional media wrapper adds 163 lines of code and only 39 lines for the product page and featured product section respectively. I think this is great tradeoff considering the factors above.

I used the Desktop image placement setting (from the Image with text section) as inspiration. I added following modifier class to the product container in both the featured-product.liquid and main-product.liquid sections. I also added shorthand padding to avoid having to declare multiple padding-left: 0; as an override - for better readability.

I manually added translations for the featured product section media_position strings for all locales.

Update (July 12 2022): Based on Lucas' comment, the Skip to product information link was removed from the featured product section.

Other considerations

Snippet?

I thought about creating a snippet for the product__media-wrapper but since they're not identical (main-product.liquid and featured-product.liquid), I decided against it as it would add more complexity to the code.

Accessibility

I updated the code to include an additional product__media-wrapper for the [Right] layout. And although this adds a lot more lines of code to the product page, it seems to make the most sense to maintain the correct DOM / tab order.

Product page: When the media position is set to [Right], the Skip to product information link will not be shown on desktop to avoid confusion. It will remain on mobile and when the media position is on the [Left].

Featured product

For the featured product section, I wanted to provide the same layout experience as the Image with text section:

Image with text section - image first/second layouts

Quick add

Part of the scope was to ensure the quick add modal applied the same media position of the product page and this is the case with this implementation. When the desktop media position is set to [Right] on the product page, the quick add modal will follow suit.

Testing steps/scenarios

• Featured product > Media > Desktop media position: switch between [Left] and [Right]
• Featured product > Ensure media is first (on top) on mobile when media position is [Left] or [Right]
• Product page > Product information > Media > Desktop media position: switch between [Left] and [Right]
• Product page > Ensure media images change when changing variants (this will only happen when a variant image has been assigned to a variant)
• Product page > Test the tabbing order with the different media layouts.
• Product page > Ensure media is first (on top) on mobile when media position is [Left] or [Right]
• Quick add > When viewing a collection click on the "Choose options" button: the layout should match the same media position setting of the product page

Demo links

• Editor

Checklist

• Added PR summary for release notes
• Notified the help.shopify.com team about updates to theme settings.
• Followed theme code principles
• Linted with Theme Check
• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility

[Oliviammarcello]

Looks great and works as expected! The focus order is off when the image is on the right side, but I am not sure if that is considered a problem? Video.

[andrewetchen]

Looks great and works as expected! The focus order is off when the image is on the right side, but I am not sure if that is considered a problem? Video.

Thanks for your review, @Oliviammarcello and thanks for pointing that out as well.

That was on my radar when I was first looking into this. I think the "Skip to product information" link that's currently on the first media image is a good solution though. Another solution would be to move the media wrapper code into a snippet so the DOM order (and tab/focus order) wouldn't be affected between the left and right desktop media position. The issue with this approach is that it would add more complexity to the code since the featured product section's media wrapper is slightly different than the one on the product page. We'd also have to add some logic to differentiate between desktop and mobile which could also further complicate things. That being said, I'm totally open to exploring this route if needed.

[Oliviammarcello]

Looks great and works as expected! The focus order is off when the image is on the right side, but I am not sure if that is considered a problem? Video.

Thanks for your review, @Oliviammarcello and thanks for pointing that out as well.

That was on my radar when I was first looking into this. I think the "Skip to product information" link that's currently on the first media image is a good solution though. Another solution would be to move the media wrapper code into a snippet so the DOM order (and tab/focus order) wouldn't be affected between the left and right desktop media position. The issue with this approach is that it would add more complexity to the code since the featured product section's media wrapper is slightly different than the one on the product page. We'd also have to add some logic to differentiate between desktop and mobile which could also further complicate things. That being said, I'm totally open to exploring this route if needed.

Good point about "Skip to product information" I'd assume that is sufficient.

[ludoboludo]

Looking good 👍
Left some nitpick comments and I'm curious to hear from Scott to see if the tab order should be something we worry about or not 🙂

[bacounts]

@andrewetchen, any updated on this PR's merge?

[andrewetchen]

@andrewetchen, any updated on this PR's merge?

Hey @bacounts, I'm hoping to get this PR merge by the end of next week 🤞🏼

[LucasLacerdaUX]

Tested on:

• Product Page
• Featured Product
• Quick Add

They all seem to work as expected! Spacing looks good too.

I just wanted to confirm and get more context on the accessibility discussion, to make sure we've validated that this tab order changes won't cause issues for certain users :D

[Oliviammarcello]

Focus states all look great and work as expected 👍

[LucasLacerdaUX]

We shouldn't have any duplicate IDs.

While most of them are not actually used to target an specific element (most of our components use a "starts with" approach ([id^="Gallery-"]) for querying), it is still a bad practice that can lead to errors.

Some of them are being associated with aria-describedby, aria-labelledby so these IDs should either be changed to have unique or we should remove the "copy" and rely solely on the original element.

I've highlighted most, if not all, elements with duplicated IDs below. Let me know if you have any questions and we can pair to go through this list :D

[ludoboludo]

Noted a couple things but the rest looks good to me.

[ludoboludo]

It's looking good to me 🤔 👍 I couldn't find any issue 🚀

[LucasLacerdaUX]

We need to do some small updates on media count.

I noticed there are duplicate IDs inside of <deferred-media> and <product-model> components, more specifically, id="Deferred-Poster-Modal-{{ media.id }}". However, they're not breaking anything yet so, not a major issue. We can refactor the component later to not use ids or the snippet to support a way to differentiate these ids.

Everything else is looking great and working as expected. Thanks for making these updates! :D

[LucasLacerdaUX]

👍 ✅

[ludoboludo]

🚀