-
Notifications
You must be signed in to change notification settings - Fork 48
Revive Enterprise Nav components #2837
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@hashicorp/hds-engineering should there be a changelog entry for AppHeader and AppSideNav? |
@shleewhite a couple of things that jump out to me:
maybe you could check the PRs that added (and then removed) all these things, to see what is missing? |
For sure, I can re-add those. The tickets to bring back enterprise nav were broken up into 2 parts (1 to revive the components and 1 to bring back the artifacts) so I wasn't sure how to break up the work. |
@shleewhite I see. Maybe it's easier to review if it's done in a single PR so we can make sure all the tests are passing, the showcase pages are working, etc? (like if it was the PR of new components). |
@shleewhite let me know when this is ready for review, I can do it |
@shleewhite I am about to review the PR. In the meantime, quick question: do you think there is a way for us to test the changes in a consumer application, let's say Cloud UI and/or Terraform? Just to make sure everything works as expected. |
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 minor comments. Overall is ready to go, up to you if you want to make changes (happy to re-approve)
</Hds::AppHeader> | ||
`); | ||
test('it renders content passed into the globalActions and utilityActions named blocks', async function (assert) { | ||
await render(hbs`<Hds::AppHeader> |
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.
[nitpick] can you check if the test files should be reformatted? I see a strange indentation here.
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.
Hmm... I checked and it seems like the linter wants it to be indented like this ¯_(ツ)_/¯
68e6ec1
to
641c8ff
Compare
📦 RC Packages PublishedLatest commit: a2bd870 Published 1 packages@hashicorp/design-system-components@4.19.0-rc-20250425201725
|
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 good! Just had a nit and one docs question.
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.
👍
📌 Summary
If merged, this PR would:
Showcase preview pages
🔗 External links
Jira ticket: HDS-4777
👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.