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

Add usa-link component #8

Merged
merged 7 commits into from
Jun 12, 2024
Merged

Add usa-link component #8

merged 7 commits into from
Jun 12, 2024

Conversation

heymatthenry
Copy link
Contributor

@heymatthenry heymatthenry commented May 21, 2024

Note

This is a basic implementation of this component which is not necessarily where it will need to be for alpha status. This relates to #9 but the work in this PR does not close that issue.

This is a basic implementation of a usa-link component. It supports both JS-required and a progressively-enhanced version. So a user can just include the component, like:

<usa-link href="https://designsystem.digital.gov">Use USWDS!</usa-link>

which will render a vanilla HTML link to the shadow DOM with the right styling (additional functionality TK). Alternatively, a user can include their own link which will render as the original HTML before JS runs (or if it doesn't run at all), and will get additional functionality if/when the script executes.

<usa-link>
    <a href="https://designsystem.digital.gov">Use USWDS!</a>
</usa-link>

In practice, that looks like this. This shows the case when JS is disabled. The first text on the page is the JS-required version, which renders as unclickable text. The second usa-link, with the plain HTML link as a child, still renders:
image

This is what the DOM looks like for these two versions of the component when JS doesn't run:
image

When JS does run, both versions are equivalent with the same styling applied to both.

@heymatthenry heymatthenry marked this pull request as ready for review June 10, 2024 15:31
@heymatthenry heymatthenry requested a review from mejiaj June 10, 2024 15:31
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Looks good, I was able to test both variants successfully. As well as tel and mailto without any issues.

I was also able to override an individual link via a CSS class.

  <style>
    :root {
      --theme-link-color: orange;
    }

    .james {
      --theme-link-color: green;
    }
  </style>

  <p>
    <usa-link href="http://designsystem.digital.gov">
      It's dangerous to go alone. Here, take this.
    </usa-link>
  </p>

  <p>
    <usa-link class="james">
      <a href="https://designsystem.digital.gov">It's dangerous to go alone. Here, take this.</a>
    </usa-link>
  </p>

  <p>
    <usa-link>
      <a href="https://google.com">A test link</a>
    </usa-link>
  </p>

Results in:

web-components/src/components/usa-link/index.js Outdated Show resolved Hide resolved
: html`<a class="usa-link" href="${this.href}"><slot></slot></a>`;
}

static styles = css`
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to establish a pattern on where styles should live.

Placing them above render() feels familiar to setting a <link rel="stylesheet"> above HTML markup. Though I can see the benefits of having styles after for convenience while developing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any benefit of having css in JS like this over having a separate styles.css sheet and pulling it in via unsafeCSS?

I see a benefit of being able to write CSS in a CSS file and keeping index.js as JS focused as possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't have any objections to keeping styles in separate files alongside the component code. Definitely helps maintainability. I'll factor them out here.

Comment on lines 34 to 38
return this.hasLinkChild()
? html`<a class="usa-link" href="${this.href}"
>${this.slottedChildren}</a
>`
: html`<a class="usa-link" href="${this.href}"><slot></slot></a>`;
Copy link
Contributor

@mejiaj mejiaj Jun 10, 2024

Choose a reason for hiding this comment

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

We could improve readability and make it easier to update if we separated these templates out. Like in the example here - Composing templates – Lit

    linkChildTemplate() {
      return html`<a class="usa-link" href="${this.href}">${this.slottedChildren}</a>`
    }
    linkSlotTemplate() {
      return html`<a class="usa-link" href="${this.href}"><slot></slot></a>`
    }

    return this.hasLinkChild()
      ? this.linkChildTemplate()
      : this.linkSlotTemplate()

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm personally a fan of this. I took a similar approach with the card component.

@heymatthenry
Copy link
Contributor Author

@mejiaj Thanks for your thoughtful feedback! I was totally onboard with your suggestions, and implemented them. I also added some JSDoc comments, along the lines of what's described here (not necessarily suggesting incorporating that tool unless it makes sense, but I've seen a lot of components doing docs with this format).

@heymatthenry heymatthenry merged commit 3e93d4c into develop Jun 12, 2024
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.

3 participants