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

feat(NcIconSvgWrapper)!: remove ID from svg #4607

Merged
merged 3 commits into from Oct 5, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Oct 4, 2023

☑️ Resolves

All material design icons from @mdi have ID attribute. It seems to be used by material design icons for web-font generation, but not needed in our apps and creates multiple duplicated IDs.

This PR removes the ID using DOM parser.

🖼️ Screenshots

🚧 Tasks

  • BREAKING CHANGE Remove ID from passed SVG
  • Add keepId prop to have a possibility to keep the ID (is it unneeded?)
  • Remove empty aria-label and aria-hidden="false" Vue 3 attributes

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Note: arria-hidden="false" doesn't add an attribute in Vue 2, but does in Vue 3.

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme added 3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Oct 4, 2023
@ShGKme ShGKme added this to the 8.0.0 milestone Oct 4, 2023
@ShGKme ShGKme self-assigned this Oct 4, 2023
Comment on lines +123 to +131
/**
* By default MDI icons have an ID on the `<svg>` element. It leads to dupliated IDs on a web-page.
* This component removes the ID on the received SVG.
* Use this prop to disable this behavior and to not remove the ID.
*/
keepId: {
type: Boolean,
default: false,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think this is needed

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@skjnldsv skjnldsv added the breaking PR that requires a new major version label Oct 5, 2023
@skjnldsv skjnldsv changed the title [BREAKING CHANGE] feat(NcIconSvgWrapper)!: remove ID from svg feat(NcIconSvgWrapper)!: remove ID from svg Oct 5, 2023
@skjnldsv skjnldsv merged commit ff8b1d9 into master Oct 5, 2023
15 checks passed
@skjnldsv skjnldsv deleted the feat/nc-icon-svg-wrapper-remove-svg-id branch October 5, 2023 05:54
@ShGKme ShGKme added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 5, 2023
@Pytal Pytal mentioned this pull request Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish accessibility Making sure we design for the widest range of people possible, including those who have disabilities breaking PR that requires a new major version feature: icon-svg-wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BITV] 9.4.1.1/0.2 On the page, there are multiple IDs that are identical - see "resourceNameFintestExplorer".
3 participants