Skip to content

Conversation

@shleewhite
Copy link
Contributor

@shleewhite shleewhite commented Aug 21, 2024

📌 Summary

If merged, this PR will add a tag or titleTag argument to the following components:

  • Accordion
  • Alert
  • ApplicationState
  • CodeBlock
  • DialogPrimitive

Note for reviewers: It is probably easiest to review this PR by commit because each component was done in a separate commit.

🛠️ Detailed description

The new argument accepts: "div" or "p", depending on which the component currently uses, and "h1" | "h2" | "h3" | "h4" | "h5" | "h6"

Accordion

  • Add titleTag argument

Alert

  • Add tag argument to Alert Title

ApplicationState

  • Update the titleTag argument to only allow: "div" | "h1" | "h2" | "h3" | "h4" | "h5" | "h6"

CodeBlock

  • Add tag argument to CodeBlock Header

DialogPrimitive

  • add titleTag argument to DialogPrimitive Header
  • update styles so changing the tag doesn't add margin

Also updates the showcase so there are examples of customized tags.

📸 Screenshots

The rendered HTML for Accordion when you pass titleTag
Screenshot 2024-08-21 at 4 08 21 PM

🔗 External links

RFC: https://go.hashi.co/rfc/ds-087
Jira ticket: HDS-3772


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

@vercel
Copy link

vercel bot commented Aug 21, 2024

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

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Aug 23, 2024 5:50pm
hds-website ✅ Ready (Inspect) Visit Preview Aug 23, 2024 5:50pm

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.

Generally looks good! I left a couple of questions and recommendations.

More generally, it would be good to add tests for the newly introduced arguments. We'd also need to update the API in website and update some examples (or add new ones), but that can be done in a separate PR if you prefer.

@shleewhite
Copy link
Contributor Author

Generally looks good! I left a couple of questions and recommendations.

More generally, it would be good to add tests for the newly introduced arguments. We'd also need to update the API in website and update some examples (or add new ones), but that can be done in a separate PR if you prefer.

@alex-ju I was planning on updating the website in a separate PR! I wanted to take a bit of time to think about how to explain / demonstrate the correct usage.

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.

Excellent work!

Copy link
Contributor

@zamoore zamoore left a comment

Choose a reason for hiding this comment

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

Great work!

SPDX-License-Identifier: MPL-2.0
}}
<div class="hds-alert__title hds-font-weight-semibold" ...attributes>{{yield}}</div>
{{! IMPORTANT: we removed any extra newlines before/after the `let` to reduce the issues with unexpected whitespaces (see https://github.com/hashicorp/design-system/pull/1652) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine (and I see this is already merged) but just to let you know, another way to remove whitespace within Ember HBS files is to use tilde symbols "~" (sometimes referred to as "squishies" in Ember)

An example of using squishies in our code base: https://github.com/hashicorp/design-system/blob/main/packages/components/src/components/hds/rich-tooltip/index.hbs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Cool! Danger of copy/pasting from older files haha, I have another branch to update the default tags for Modal/Flyout so maybe I can clean that up in that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants