Skip to content

Conversation

@didoo
Copy link
Contributor

@didoo didoo commented Apr 14, 2022

📌 Summary

While working on the HDS components, me and @MelSumner we've found ourselves writing over and over the same "logic" to handle the different ways to declare a link vs a LinkTo vs a button in Ember (here one of the last examples).

This is the TLDR of the "logic" (this is pseudo-code, the actual implementation in code is slightly different)

{{#if @route}}
  {{#if @isRouteExternal}}
    <LinkToExternal>
  {{else}}
    <LinkTo>
  {{/if}}
{{else if @href}}
  {{#if @isHrefExternal}}
    <a target="_blank" rel="noopener noreferrer">
  {{else}}
    <a>
  {{/if}}
{{else}}
  <button/>
{{/if}}

The intent of this PR is to create a generic, reusable, "utility" component (almost headless) to centralize this logic in a single place, and abstract it away from the actual UI components.

Where it will be used?

At the moment we expect to use it in these components:

  • button
  • dropdown/list-item/interactive
  • link-standalone
  • link-inline (coming soon)

but is likely that most of all the future interactive components/elements will use it as well.

...but wait, why use @href as argument/prop and not just href attribute?

We can't support the direct use of the href HTML attribute because we need to rely on the @href Ember argument to differentiate between different types of generated output (see the "logic" above).

🛠️ Detailed description

In this PR I have:

  • added a genericHds:Interactive component
    • under one single component it handles the generation of <button> / <a> / <a target="_blank" rel="noopener noreferrer"> / <LinkTo> / <LinkToExternal> conditionally, depending on the @href / @route / @isHrefExternal / @isRouteExternal arguments passed
    • no styling applied, is just a "utility" component that abstract the logic above in a single place (we are using it over and over in our components) - the styling will be applied via CSS classes that will be applied to the instance (and its children) in the context where it's used
  • updated the dropdown/list-item/interactive to use the new Hds::Interactive generic component
  • added the documentation page (preview here)
  • added the integration tests

🤔 Things to decide/discuss


👀 How to review

👉 Review by files changed

Reviewer's checklist:

  • +1 Percy if applicable

💬 Please consider using conventional comments when reviewing this PR.

@changeset-bot
Copy link

changeset-bot bot commented Apr 14, 2022

⚠️ No Changeset found

Latest commit: d0aa0d6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Apr 14, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

hds-flight-website – ./

🔍 Inspect: https://vercel.com/hashicorp/hds-flight-website/BG258DcyyTwsPk7SrRh9ciDfkVWE
✅ Preview: https://hds-flight-website-git-spike-interactive-gener-40fe9c-hashicorp.vercel.app

hds-components – ./

🔍 Inspect: https://vercel.com/hashicorp/hds-components/G1P8XHqM6wFsC1VKGjXhtywiSsw6
✅ Preview: https://hds-components-git-spike-interactive-generic-c-6d4c05-hashicorp.vercel.app

@didoo didoo changed the base branch from main to spike-BUTTON-LINK-CTA-main April 14, 2022 17:26
@didoo didoo changed the title [WIP] Spike interactive generic component [WIP] Spike interactive generic component (01) Apr 14, 2022
@didoo didoo changed the title [WIP] Spike interactive generic component (01) [WIP] "Interactive" generic component (01) Apr 14, 2022
@vercel vercel bot temporarily deployed to Preview – hds-flight-website April 14, 2022 18:19 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components April 14, 2022 18:19 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components April 14, 2022 18:27 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website April 14, 2022 18:27 Inactive
@didoo didoo marked this pull request as draft April 14, 2022 18:30
@vercel
Copy link

vercel bot commented Apr 19, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
hds-components ✅ Ready (Inspect) Visit Preview May 9, 2022 at 10:14AM (UTC)
hds-flight-website ✅ Ready (Inspect) Visit Preview May 9, 2022 at 10:14AM (UTC)

@vercel vercel bot temporarily deployed to Preview – hds-flight-website April 19, 2022 13:40 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components April 19, 2022 13:41 Inactive
it was triggering a lint-error: Do not create empty backing classes for Glimmer components  ember/no-empty-glimmer-component-classes
@vercel vercel bot temporarily deployed to Preview – hds-flight-website April 20, 2022 12:04 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components April 20, 2022 12:04 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website April 20, 2022 12:17 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components April 20, 2022 12:18 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components April 20, 2022 12:22 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website April 20, 2022 12:22 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components April 27, 2022 10:34 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website April 27, 2022 10:34 Inactive
@didoo
Copy link
Contributor Author

didoo commented Apr 27, 2022

@MelSumner as discussed yesterday, I have added the handling of the @isHrefExternal (defaults to true) for the <a> links. You can see the commits here e12dd44 and here 3544a8d.

didoo added 2 commits May 5, 2022 19:03
# Conflicts:
#	packages/components/addon/components/hds/dropdown/list-item.hbs
@vercel vercel bot temporarily deployed to Preview – hds-flight-website May 5, 2022 18:06 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components May 5, 2022 18:06 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website May 5, 2022 18:08 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components May 5, 2022 18:08 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website May 5, 2022 18:59 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components May 5, 2022 18:59 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website May 5, 2022 19:01 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components May 5, 2022 19:01 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components May 5, 2022 19:12 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website May 5, 2022 19:12 Inactive
@didoo didoo requested review from Dhaulagiri, MelSumner and alex-ju May 5, 2022 19:18
Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

As far as my understanding of LinkTo and LinkToExternal goes at the moment, this makes sense to me. No suggestions for improvements from my side, I'm afraid.

</a>
{{/if}}
{{else}}
<button type="button" ...attributes>
Copy link
Member

Choose a reason for hiding this comment

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

I like that we set this by default to type="button" to prevent accidental form submissions

@vercel vercel bot temporarily deployed to Preview – hds-flight-website May 9, 2022 10:14 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components May 9, 2022 10:14 Inactive
Copy link
Contributor

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

Tentative about this one but let's explore how it works!

@didoo didoo merged commit f43fae3 into spike-BUTTON-LINK-CTA-main May 11, 2022
@didoo didoo deleted the spike-interactive-generic-component branch May 11, 2022 09:44
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.

5 participants