vtex-apps / product-summary

VTEX Product Summary app
11 stars 51 forks source link

Add a second criteria to match the hover image beyond only by its label #283

Closed imdanielpiva closed 3 years ago

imdanielpiva commented 4 years ago

What problem is this solving?

This PR aims to solve two problems:

1) When using a hover image the main image continues appearing, so if the hover image has any transparency the main image will also appear.

2) Currently there's no way to set dynamically the hover image by label, it has to be a static and fixed string on the product-summary-image instance but when you're working with a dynamic list of products which each one has a different pattern of image labels set on the Admin's Catalog then it will not work.

So the first problem is pretty simple so I just added one more CSS handle (mainImageHovered) and another CSS rule to control its opacity when it's hovered.

For the second problem I came up with a new criteria to match the hover image which is by index. So the product has a list of images and you would set which position of that list that you will want to pick the hover image. However the hoverImageLabel continues working the way it does now and it is the default criteria ("hoverImageCriteria": "label")

Something like:

  "product-summary-image#by-index": {
    "props": {
      // it's gonna pick the third image of the list
      "hoverImageIndex": 3,
      "hoverImageCriteria": "index"
    }
  }

How to test it?

Workspace

Screenshots or example usage:

gif

How does this PR make you feel? :link:

vtex-io-ci-cd[bot] commented 4 years ago

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

vtex-io-docs-bot[bot] commented 4 years ago

Beep boop :robot:

Thank you so much for keeping our documentation up-to-date :heart:

imdanielpiva commented 3 years ago

Actually, I'm unable to browse the workspace provided in the PR. The code looks good to me, tho. Could you fix that, please?

Yes, sure, I'll fix the workspace!

So about the API design can I change it to the @klzns' suggestion as the draft bellow?

type HoverImage = {
  label?: string
  index?: number
  criteria: 'index' | 'label'
}
icazevedo commented 3 years ago

Actually, I'm unable to browse the workspace provided in the PR. The code looks good to me, tho. Could you fix that, please?

Yes, sure, I'll fix the workspace!

So about the API design can I change it to the @klzns' suggestion as the draft bellow?

type HoverImage = {
  label?: string
  index?: number
  criteria: 'index' | 'label'
}

It would be great!

imdanielpiva commented 3 years ago

Hey @icazevedo and @klzns both the workspace and the PR are updated!

Important Notes

icazevedo commented 3 years ago

@imdanielpiva we couldn't afford to make it a breaking change. To speed things up, I thought about contributing to your PR, but I don't have the permissions to push my commits since you are merging from master.

I've adapted your solution to be backwards compatible in this PR. We're also refactoring the code to Typescript, so I've started from a branch where that's already done. The changes referring to this PR are present on the last commit of the PR's branch.

As soon as the TS version goes live, the code should be ready to be merged and then we can add you as a contributor to the repo. Thanks for the submission!