fix(ui-shell): change the way to generate unique ID #4291
Conversation
|
Deploy preview for carbon-elements ready! Built with commit b4aef68 |
|
Deploy preview for the-carbon-components ready! Built with commit b4aef68 https://deploy-preview-4291--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react failed. Built with commit b4aef68 https://app.netlify.com/sites/carbon-components-react/deploys/5da110234398e500081892b0 |
| launcher.id = `__carbon-product-switcher-launcher-${Math.random() | ||
| .toString(36) | ||
| .substr(2)}`; | ||
| launcher.id = `__carbon-product-switcher-launcher-${seq++}`; |
There was a problem hiding this comment.
FYI, ids must start with a letter in order to be valid HTML. If this is an id that get's added to the HTML I would suggest removing the underscores.
There was a problem hiding this comment.
Is it more-so that it was a requirement in HTML4? And they suggest doing it for backwards compat
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id
Note: Using characters except ASCII letters, digits, '_', '-' and '.' may cause compatibility problems, as they weren't allowed in HTML 4. Though this restriction has been lifted in HTML5, an ID should start with a letter for compatibility.
There was a problem hiding this comment.
Given the point seems for older browsers probably we'd go for it as-is for now.
|
@emyarod Updated the original issue; Seems that none of the reports actually affect their app. The random ID one (this PR) is just for pre-caution. |
Closes #4288.
Changelog
Changed
Math.random()to sequence number to address the presumable concern with security.Testing / Reviewing
Testing should make sure our vanilla product switcher is not broken.