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

Prevent multiple <a> tags in progressively enhanced usa-link #40

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

bjmfactory
Copy link

@bjmfactory bjmfactory commented Jul 24, 2024

Summary

Prevented multiple tags in progressively enhanced usa-link Fixed an issue in which there are nested tags in the usa-link when a childLink is provided.

Problem statement

We want only one tag in the output HTML when we use the progressively enhanced usa-link.

Desired state:

<usa-link>
  #shadow-root (open)
  <!--?lit$464523568$-->
  <a href="http://designsystem.digital.gov" class="usa-link"><!--?lit$019672011$-->It's dangerous to go alone. Here, take this.</a>
  <!--?-->
</usa-link>

The actual state

<usa-link>
  #shadow-root (open)
  <a href="http://designsystem.digital.gov"><!--?lit$464523568$-->It's dangerous to go alone. Here, take this.</a>
  <!---->
  <a class="usa-link" href="http://designsystem.digital.gov/"><slot></slot></a>
</usa-link>

Consequences of remaining in the current state
It is invalid HTML.

Solution

I removed the wrapper tag and instead added the usa-link class to the existing link that is passed as a child. It's quite possible I did not follow Web Component best practices or your company best practices, or that there are other things I'm not considering, so there are lots of limitations :)

Testing and review

I added a test that asserts that there is only one tag.

this.shadowRoot.appendChild(this.slottedChildren);
return true;
Copy link
Author

Choose a reason for hiding this comment

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

Without this line, I was observing the slot template is always rendered and the child template is never rendered.

Copy link
Author

Choose a reason for hiding this comment

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

When I ran npm run dev I got an error that said to remove node_modules and package-lock.json and re-run npm install. I'm not sure what the root cause of that is, but there was some error about Rollup I think.

Copy link
Author

Choose a reason for hiding this comment

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

I'm happy to remove this change if you like the other fix but don't want to touch package-lock.json at the moment.

@@ -42,4 +42,9 @@ describe("progressively enhanced usa-link component", async () => {
it("should render link with component markup", () => {
expect(getInsideLink().className).toContain("usa-link");
});

it("should render only one <a> tag", () => {
Copy link
Author

Choose a reason for hiding this comment

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

This test successfully captures the current bug on develop. If you add it on develop it should fail with a 2 does not equal 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant