feat(vital-carousel): PDP Carousel #2204
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@hvanyo is attempting to deploy a commit to the JNJ Demo Account Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for creating a pull request on GitHub! We appreciate your contribution to our repository. In order to enable our tests to run on your changes, we kindly ask that you wait until one of our repository members adds the ready-to-test label to your pull request. (Please request on the GitHub issue you are addressing.) This will ensure that our testing process runs smoothly and we can quickly verify the changes you've made. Thank you for your understanding and we look forward to reviewing your changes soon! |
| @@ -62,6 +71,14 @@ export const sliderSimpleInit = (sliderSimpleElement: HTMLElement) => { | |||
| const NumThumbs = 4; | |||
| for (const button of Array.from(buttonsThumbs)) { | |||
There was a problem hiding this comment.
Shouldn't this code be under the if (sliderThumbCarousel.length !== 0) {...} condition above? There is no need to add controls if there are no thumbs, right? Or is it still has to present?
Also if the condition above is not met, then this line might error since buttonsThumbs will be undefined in Array.from(buttonsThumbs)
Array.from(buttonsThumbs) could also be saved to a var and reused for a tiny speed bump: https://github.com/johnsonandjohnson/Bodiless-JS/pull/2204/files#diff-6c6a78fcbd165e433925aac71d373a5d6284eabbdf275a4825969bc59913afe7R52.
There was a problem hiding this comment.
@VancheeZze I think this code is under the condition in line 46.
The typescript version only runs on edit so performance doesn't matter.
The non-typescript javascript version runs on static and reuses buttonsThumbs array with typescript cast.
I learned the lesson the hardway that browser doesn't like to run typescript'ed javascript :)
| @@ -103,14 +117,15 @@ const Default = asPDPTemplateToken({ | |||
| }, | |||
| Layout: { | |||
| ContentWrapper: 'flex flex-wrap', | |||
| ProductImageWrapper: 'flex justify-center w-full lg:w-1/2', | |||
| ProductCarouselWrapper: 'lg:w-1/2', | |||
There was a problem hiding this comment.
Should this ProductCarouselWrapper be hidden on mobile and only be visible on desktop — opposite from the MobileProductCarouselWrapper below?
There was a problem hiding this comment.
@VancheeZze I was letting component tokens in line 92 / 99 do the responsive hiding. I had to add the lg:hidden on line 121 though otherwise it screwed up the flex box lg w-1/2 on desktop. As i made the change, I pondered if the way we should do this was by wrapper on template or by the reponsive token.. Opinions?
Changes
Test Instructions
Related Issues