Fix duplicate IDs occurrence in product options in certain situations #1223
Conversation
|
Autotagging @bigcommerce/storefront-team @davidchin |
|
@junedkazi sorry, I messed up my squash as there were a couple more commits to the master while I was working on it, it was quicker to do another PR. |
There was a problem hiding this comment.
Thanks @flair-duncan Code changes LGTM 👍. We will need to do some testing internally before we merge this one.
|
@flair-duncan do you have an example of duplicate IDs you can show, or some replication steps to get to such a state? This might be something we want to fix on the backend... |
|
@bookernath we noticed this occurring on our own theme, and traced it back to the option components in Cornerstone. If you look at our theme store demo site on this page you can replicate the ID clash by clicking the color swatches. Clicking the black swatch will select the "Chest Size" option. Both options have a duplicate ID, in this case I've used this same data with Cornerstone and the same thing happens. If you want me to create an API key so you can use that store data, let me know. 👍 |
|
@Ubersmake Can you test this? |
|
@flair-duncan sorry for the delay can you rebase this PR & move the change log into the new draft section please. |
|
@flair-duncan looks like the rebase has gone wrong. |
|
@junedkazi hmm, I'll sort it out over the weekend... |
|
@flair-duncan this looks good to me. Can you please squash your commits into a single one please. |
* Modernizr is a blocking script that has very little benefit for us. Removing this script improves page responsiveness. * We used Modernizr via the css classes `csscolumns`, `flexbox`, `objectfit`, and `js`. * The `csscolumns` class was used for productMasonry for older browsers, but since then all of our supported browsers support css columns. There is one small problem with Firefox: it does not support break-inside, but it does support page-break-inside, which does the same thing. See https://caniuse.com/#feat=multicolumn * The `flexbox` class is unused * The `objectfit` class is used by carousel. Reimplmented without using Modernizer. The previous implmentation put a background image on the wrapper div and then hid it if object-fit is supported. The new implementation does not put the background image on the wrapper div by default, but instead assumes that your browser supports object-fit (all ours do except for IE). For IE, we use javascript to copy the image source from the image tag to the background image of the wrapper div and then hide the image. * The `js` class is used for a few things in css, so mimic this behavior with a simple inline script.
* Add wrapper div for hero carousel that has the appropriate height for the image to be displayed after lazy loading. * This improves frontend performance because the browser will be less likely to load images below the fold immediately.
* Add dns prefetch & preconnect to core resources that we know we will need -- fonts, cdn, jirafe, etc
* Upgrade to webpack 4 * Clean up stencil bootstrap * Simplify interface for PageManager and get rid of async library dependency * Get rid of html5-history-api library dependency (no longer needed) * Get rid of fastclick library dependency (no longer needed) * Upgrade several babel dependencies * Separate webpack config into common, dev, prod, and add npm scripts to build * Get rid of minifications in babel loader, instead rely on webpack * Get rid of LoaderOptionsPlugin * Get rid of CommonsChunkPlugin (webpack 4 will automaticaly do this for prod)
* Use svg-injector to fetch the svg icon sprite rather than embed it directly on the page. This improves frontend performance because the icons will be cached after the first request and we reduce the page weight by about 28kb. * Move location of generated svg so it is retrievable by the frontend through the CDN. * Update svgstore grunt task
* Reduce the default number of featured, new, and bestselling products on the home page. * These can always be updated in Theme Editor, but the defaults don't need to be so high.
* Use laziest possible setting for images
* Use `resourceHints` helper from paper 2.0.8, which looks up configured cdn and font providers.
…and moving some tags to the appropriate div
… reviews are not present
setting for skus. We were not properly switching back and forth
between skus that were above and below their own respective
threshholds.
…es for simple products
and make it strike through.
In certain circumstances elements end up with the same ID causing a clash, stopping some options being selected Changes as requested on PR#1215 Change _ to - in input-checkbox component Open correct product page tabs when URL refers to them Comment and clean up product-details.js Changes as requested on PR#1215 Change _ to - in input-checkbox component Update product-details.js Fix duplicate IDs occurrence in product options in certain situations Fixing
|
@junedkazi I've got absolutely no idea why it's just done that, I successfully rebased and squashed at my end... I'm just going to create a clean PR for you to save time. Sorry 😞 |
|
@flair-duncan did u get a chance to look into this issue ? |
|
closed in favor of #1261 |
What?
In certain circumstances, product option elements end up with the same ID causing a clash, stopping some options being selected.
Each option component has been given a descriptive ID to prevent the clash. As the add to cart action relies on the name attribute, this does not interfere with the cart system.
Screenshots
Video of issue before changes
Video after changes