Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove "hover" option for image zoom #2283

Merged
merged 1 commit into from
Feb 3, 2023
Merged

Conversation

stufreen
Copy link
Contributor

@stufreen stufreen commented Feb 3, 2023

PR Summary:

Removes the "hover" option from the Image Zoom settings.

Why are these changes introduced?

We discovered a bug where transparent PNG files would show the thumbnail image below the hovered image. We will remove the option for now and fix for a future release.

What approach did you take?

Comments out the option and the label (which is only relevant for hover).

Decision log

# Decision Alternatives Rationale Downsides
1 Remove hover option from select setting Remove all code related to image zoom, not just the option We'll be releasing this feature as a fast follow to this release. Leaves some dead code in the codebase until the feature is released

Visual impact on existing themes

NA

Testing steps/scenarios

  • In the Editor, open the product template. Go to main product section.
  • Ensure the "Image Zoom" setting includes two options (lightbox and none) and works as expected.

Demo links

Checklist

@stufreen stufreen marked this pull request as ready for review February 3, 2023 14:38
@@ -1424,18 +1424,13 @@
"value": "lightbox",
"label": "t:sections.main-product.settings.image_zoom.options__1.label"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be removing the translations too? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we keep the translations. Similar to the dead code, it just makes our lives easier without causing any negatives.

The translation platform should have some memory of previous translations so that won't cause issues with time whenever we revisit it on the next release, BUT translation conflicts are a pain to deal with so let's just keep it there. There's not much negative in having some extra unused strings.

@@ -1424,18 +1424,13 @@
"value": "lightbox",
"label": "t:sections.main-product.settings.image_zoom.options__1.label"
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this can be commented out or removed for now too 🤔 https://screenshot.click/03-58-akjn2-e2l2x.png

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say to keep it. It's okay if we're revisiting it soon.
As long as that condition will never be true, it's okay to leave some dead code in to save us some time when we fix this feature.

Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@stufreen stufreen merged commit e41e190 into main Feb 3, 2023
@stufreen stufreen deleted the stu/remove-image-zoom branch February 3, 2023 16:03
uniorusa added a commit to uniorusa/unior-usa-b2b that referenced this pull request Feb 3, 2023
Remove "hover" option for image zoom (Shopify#2283)
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants