-
Notifications
You must be signed in to change notification settings - Fork 49
Add support for Text in the AppHeader
#2951
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
AppHeader
AppHeader
…dding instead to support variable length string content
…sktop to the showcase, along with examples of child components
83b6e75
to
3a0055c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good! I added a few comments on simplifying some logic and a few other minor recommended changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, but nothing major/blockign
packages/components/src/components/hds/app-header/home-link.hbs
Outdated
Show resolved
Hide resolved
… default and simplified logic
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments/todos
get icon(): HdsIconSignature['Args']['name'] { | ||
const { icon } = this.args; | ||
|
||
assert('@icon name must be provided', icon !== undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[issue] as a consumer, I see this error in the devtool console, and I don't know which component threw the error or where is coming from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a more explicit assertion for this
showcase/tests/integration/components/hds/app-header/home-link-test.js
Outdated
Show resolved
Hide resolved
showcase/tests/integration/components/hds/app-header/home-link-test.js
Outdated
Show resolved
Hide resolved
showcase/tests/integration/components/hds/app-header/home-link-test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few more comments/todos
@color="var(--token-color-boundary-brand)" | ||
@href="#" | ||
/> | ||
</div> | ||
</SF.Item> | ||
</Shw::Flex> | ||
|
||
<Shw::Text::H4>Text</Shw::Text::H4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[todo] For consistency with the same title above?
<Shw::Text::H4>Text</Shw::Text::H4> | |
<Shw::Text::H4>With text</Shw::Text::H4> |
<Shw::Flex as |SF|> | ||
<SF.Item> | ||
<div class="hds-app-header"> | ||
<Hds::AppHeader::HomeLink @icon="hashicorp" @text="HashiCorp" @href="#" @title="Admin UI" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[todo] I think this one is missing the @isIconOnly={{false}}
?
<Hds::AppHeader::HomeLink @icon="hashicorp" @text="HashiCorp" @href="#" @title="Admin UI" /> | |
<Hds::AppHeader::HomeLink @icon="hashicorp" @isIconOnly={{false}} @text="HashiCorp" @href="#" @title="Admin UI" /> |
|
||
<SF.Item> | ||
<div class="hds-app-header"> | ||
<Hds::AppHeader::HomeLink @icon="terraform" @text="Terraform" @href="#" @title="Terraform Admin Console" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[todo] I think this one is missing the @isIconOnly={{false}}
?
<Hds::AppHeader::HomeLink @icon="terraform" @text="Terraform" @href="#" @title="Terraform Admin Console" /> | |
<Hds::AppHeader::HomeLink @icon="terraform" @isIconOnly={{false}} @text="Terraform" @href="#" @title="Terraform Admin Console" /> |
|
||
<Shw::Text::H3>With text</Shw::Text::H3> | ||
|
||
<Shw::Grid @columns={{1}} {{style gap="2rem"}} as |SG|> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[general consideration] no need to change it here, but in general it would be better to avoid using a grid to achieve a simple vertical stacked layout, and and use <Shw::Grid @direction="column">
instead (CSS grid comes with (rare) potential side effects, that we want to avoid)
📌 Summary
Adds support for a
text
argument within the HomeLink of theAppHeader
. Refactors the HomeLink to match the logic of the<Hds::Button>
:ariaLabel
argumentisIconOnly
argument andtext
argumenttext
argument to thearia-label
of the Home Link whenisIconOnly={{true}}
Showcase: https://hds-showcase-git-jt-application-titlehds-4866-hashicorp.vercel.app/components/app-header
📸 Screenshots
This first example is representative of the TFE Admin Console app that is being spun up, the other two are speculative examples for how the component could be used elsewhere.
Example states with Application Title

🔗 External links
Jira ticket: HDS-4866
Figma file
👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.
📋 PCI review checklist
Examples of changes to controls include access controls, encryption, logging, etc.
Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.