Move both thumbnail selection classes together when swiping - #226
Draft
boo-code wants to merge 1 commit into
Draft
Move both thumbnail selection classes together when swiping#226boo-code wants to merge 1 commit into
boo-code wants to merge 1 commit into
Conversation
The default thumbnail is rendered with "selected" and "js-thumb-selected", and themeSelectors.product.selected matches either of them. Swiping removed only "selected" from the old thumbnail, so it kept "js-thumb-selected" and went on matching that selector. The next swipe therefore read a set of thumbnails rather than one, took .closest().next() on all of them and marked every result selected, so the highlight spread across the strip and stepping through the images stopped working part way. Counting the matched thumbnails over three swipes of a four image product, before and after: shipped 2, 3, 4 matched 1, 2, 3 highlighted fixed 1, 1, 1 matched 1, 1, 1 highlighted, advancing 2, 3, 4 See PrestaShop/PrestaShop#33782
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
selectedandjs-thumb-selected(product-cover-thumbnails.tpl:74), andthemeSelectors.product.selectedis'.selected, .js-thumb-selected'— it matches either.swipe()removed onlyselectedfrom the outgoing thumbnail, so that thumbnail keptjs-thumb-selectedand carried on matching the selector.Every later swipe therefore read a set of thumbnails instead of one, called
.closest().next()on all of them and marked each result selected. That is both reported symptoms: the highlight spreads across the strip, and stepping through the images stops working part way.Measured
The two lines that change are pure class handling, so the mechanism was measured directly: the markup
product-cover-thumbnails.tplemits for a four-image product, the selectors fromselectors.jsverbatim, and the same
.closest()/.next()walkswipe()performs, run three times.The left column is the defect: the matched set grows by one on every swipe, because each outgoing
thumbnail keeps
js-thumb-selected. The right-hand run advances one image at a time, which is theintended behaviour.
Scope
Only the class pair is touched.
js-thumb-selectedhas exactly three references in the theme — theselector, the template, and these two lines — so nothing else depends on it being left behind:
The click handler above shares the same
swipe()function, so it is fixed by the same two lines.Checks
npx eslint -c .eslintrc.js js/product.jsexits 0.assets/is gitignored in this repository, so norebuilt bundle belongs in the diff.
Verification limit
Measured on the real markup and selectors with the real DOM operations, not by swiping on a device — the
touch gesture is delivered by
jquery.touchSwipe, which only decides whenswipe()is called, notwhat it does. The class bookkeeping inside it is what was wrong and what is measured above.