Skip to content

USWDS: Replaced deprecated xlink:href code with recommended use of href#6165

Merged
thisisdano merged 11 commits into
developfrom
rc-xlink-deprecated
Nov 22, 2024
Merged

USWDS: Replaced deprecated xlink:href code with recommended use of href#6165
thisisdano merged 11 commits into
developfrom
rc-xlink-deprecated

Conversation

@CTGM-Bixal

@CTGM-Bixal CTGM-Bixal commented Oct 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

All xlink:href instances have be replaced with href within the repo in order to account for the deprecation of xlink:href.

Breaking change

Indicate if this update is a breaking change with one of the following statements:
This is not a breaking change.

Related issue

Closes #6088

Related pull requests

USWDS-Site: Replaced deprecated xlink:href code with recommended use of href #2926

Preview link

Preview link:
USWDS Storybook Icon

Problem statement

The USWDS repository currently contains numerous instances of xlink:href, which is a deprecated attribute that may cause compatibility issues/errors.

Solution

The USWDSrepository has been updated to use the href attribute instead of the deprecated xlink:href attribute.

Markup Change

Users should update their xlink:href references to href.

Testing and review

  1. Open up local version of USWDS Storybook or preview link
  2. Ensure icons are working as expected
  3. Using dev tools, ensure the <use> no longer uses xlink:href and uses href instead when inspecting an icon

@CTGM-Bixal CTGM-Bixal requested review from amyleadem, cathybaptista, mahoneycm and mejiaj and removed request for amyleadem October 31, 2024 19:35
@CTGM-Bixal CTGM-Bixal marked this pull request as ready for review October 31, 2024 20:12
mahoneycm
mahoneycm suggested changes Oct 31, 2024
View reviewed changes

@mahoneycm mahoneycm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small changes requested.

Also, do you know if we're safe to remove xmlns:xlink from the svg's as well? Or do we need to keep them?

Comment thread packages/usa-input-prefix-suffix/src/usa-input-prefix.twig Outdated
Comment thread packages/uswds-core/src/js/polyfills/svg4everybody.js Outdated
Comment thread packages/uswds-core/src/js/polyfills/svg4everybody.js Outdated
@CTGM-Bixal CTGM-Bixal requested a review from mahoneycm November 1, 2024 20:30
amyleadem
amyleadem previously approved these changes Nov 1, 2024
View reviewed changes

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good. Thanks!

I successfully performed the following tests:

  • Confirm all xlink:href references have been updated to href
  • Confirm icons still display as expected in Safari, Firefox, Chrome

Can you add a changelog PR for this change?
Additionally, can you update the PR description by adding a "Markup change" section with a note that says users should update their xlink:href references to href?

mahoneycm
mahoneycm reviewed Nov 4, 2024
View reviewed changes
Comment thread packages/usa-input-prefix-suffix/src/usa-input-prefix.twig
CTGM-Bixal
CTGM-Bixal commented Nov 5, 2024
View reviewed changes
Comment thread packages/usa-modal/src/test/template.html Outdated
mejiaj
mejiaj reviewed Nov 5, 2024
View reviewed changes

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall, a few minor comments.

Comment thread packages/usa-collection/src/usa-collection--only-headers.twig
Comment thread packages/usa-modal/src/test/template.html Outdated
Comment thread packages/usa-form/src/test/test-pattern/test-usa-form.twig Outdated
@mejiaj mejiaj mentioned this pull request Nov 6, 2024
Open
@CTGM-Bixal CTGM-Bixal mentioned this pull request Nov 6, 2024
Merged
@CTGM-Bixal CTGM-Bixal requested a review from mejiaj November 7, 2024 17:18
mejiaj
mejiaj suggested changes Nov 7, 2024
View reviewed changes

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@RachelCorsino some small issues related to previous merge conflicts. Can you take a look? Please reach out if anything is unclear.

Comment thread packages/usa-checkbox/src/img/checkbox-indeterminate.svg
Comment thread packages/usa-checkbox/src/img/checkbox-indeterminate-alt.svg
Comment thread packages/uswds-core/src/js/polyfills/svg4everybody.js Outdated
@CTGM-Bixal CTGM-Bixal requested a review from a team as a code owner November 7, 2024 22:25
@CTGM-Bixal CTGM-Bixal requested a review from mejiaj November 7, 2024 23:19
mejiaj
mejiaj approved these changes Nov 12, 2024
View reviewed changes

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, added video showing the loader icon to confirm there aren't regressions.

Comment thread packages/usa-file-input/src/img/loader.svg

@mejiaj mejiaj Nov 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No regressions found, sharing for awareness.

loader-icon-2024-11-12.at.9.51.13.AM.mov

mahoneycm
mahoneycm approved these changes Nov 14, 2024
View reviewed changes

@mahoneycm mahoneycm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks @RachelCorsino !

@amyleadem amyleadem requested a review from thisisdano November 14, 2024 19:47
thisisdano
thisisdano approved these changes Nov 22, 2024
View reviewed changes

@thisisdano thisisdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

@thisisdano thisisdano merged commit c774a84 into develop Nov 22, 2024
@thisisdano thisisdano deleted the rc-xlink-deprecated branch November 22, 2024 17:51
@amyleadem amyleadem mentioned this pull request Nov 25, 2024
Merged
21 tasks
@amyleadem amyleadem mentioned this pull request Dec 9, 2024
Closed
@thisisdano thisisdano mentioned this pull request Dec 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cathybaptista cathybaptista Awaiting requested review from cathybaptista
@amyleadem amyleadem Awaiting requested review from amyleadem
3 more reviewers
@mejiaj mejiaj mejiaj approved these changes
@thisisdano thisisdano thisisdano approved these changes
@mahoneycm mahoneycm mahoneycm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

USWDS - Bug: xlink:href deprecated

5 participants

@CTGM-Bixal @mejiaj @thisisdano @mahoneycm @amyleadem