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

Have external-link component use the icon component rather than hard-coding the svg #2088

Closed
kruncher opened this issue Mar 24, 2022 · 2 comments · Fixed by #3123
Closed
Assignees
Labels
Low Priority Tech improvements Tech debt, cleanup, code standardisation etc.

Comments

@kruncher
Copy link
Contributor

kruncher commented Mar 24, 2022

To workaround some sort of spacing issue the external-link component has the icon hard-coded as an svg directly into the template:

{# Icon needed to be directly put here instead of calling macro to solve an issue with extra space on the end of links being underlined #}

Instead the icon component should be used and adjusted as needed so that it works in this situation.

Benefits:

  • Better quality unit test.
  • Avoid duplication of svg data in generated bundle.
  • One source of truth for the "external-link" icon.
@kruncher
Copy link
Contributor Author

The macro tests should be updated to use the new templateFaker().spy('icons') functionality (see macro tests in button for an example on how to do this).

@yatesn yatesn added the Tech improvements Tech debt, cleanup, code standardisation etc. label Apr 7, 2022
@yatesn
Copy link

yatesn commented Nov 16, 2023

This issue needs to be solved first #2873

@adi-unni adi-unni self-assigned this Apr 3, 2024
@adi-unni adi-unni mentioned this issue Apr 8, 2024
2 tasks
@adi-unni adi-unni linked a pull request Apr 8, 2024 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Low Priority Tech improvements Tech debt, cleanup, code standardisation etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants