Add product card image to lightbox and enable product links#16418
Add product card image to lightbox and enable product links#16418rcrphillips wants to merge 9 commits into
Conversation
CI's check-schemas gate failed because the schemas were left stale after ProductImage gained an optional `position` field for lightbox support.
… the buttons and text.
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
rcrphillips
added
dotcom-rendering
maintenance
github-actions
Bot
removed
the
run_chromatic
| xCustComponentId="lightbox" | ||
| themeOverrides={{ | ||
| backgroundTertiary: | ||
| palette.neutral[100], |
There was a problem hiding this comment.
Shouldn't this just go in the current theme object?
There was a problem hiding this comment.
Given that the lightbox is always a dark background, we want to have the overrides.
There was a problem hiding this comment.
I guess my question is more - is the background of the secondary button in the leftCol card deliberately transparent or should it just be white like this?
| * Any elements that are off screen, such as caption links for images that are not | ||
| * currently showing, are ignored | ||
| */ | ||
| const getTabbableElements = ( |
There was a problem hiding this comment.
are the product buttons tabbable?
There was a problem hiding this comment.
They are, but the tabbing does actually stop working so will fix that.
| */ | ||
| const FILTER_PAGE_ID_PREFIXES = ['thefilter/', 'thefilter-us/']; | ||
|
|
||
| export const isFilterPageId = (pageId: string): boolean => |
There was a problem hiding this comment.
For some other elements we are checking the tags for "tracking/commissioningdesk/the-filter" or "tracking/commissioningdesk/filter-us".
But - why check if it's a Filter article? Shouldn't the presence of ProductBlockElement be enough to tell us that?
There was a problem hiding this comment.
In the future we might wannt to enable product elements to exist on non-filter content. However, we would want to only narrow the 'don't show info functionality' to filter articles. Therefore, I think it still makes sense.
…dian/dotcom-rendering into rp-product-element-lightbox
andrewHEguardian
left a comment
andrewHEguardian
left a comment
There was a problem hiding this comment.
👍 but let's wait til #16202 is in we can test the carousel images
2 participants
Adds the ability for images within product cards to be show within a lightbox as well as main product images. This then also pulls through the product links, giving readers another option for clicking through to a merchants site. Note that the button styling is different between the main column product buttons and the left col product buttons because of a mismatch in the composer data model. We will need to update this at a later point.
