Skip to content

Conversation

shleewhite
Copy link
Contributor

@shleewhite shleewhite commented Apr 22, 2025

📌 Summary

If merged, this PR would:

  • deprecates SideNav component
  • stop excluding app header and app side nav in the rollup
  • enable the app header and app side nav tests
  • bring back AppHeader and AppSideNav to the showcase site

Showcase preview pages

🔗 External links

Jira ticket: HDS-4777


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Apr 22, 2025

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

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Apr 25, 2025 9:03pm
hds-website ✅ Ready (Inspect) Visit Preview Apr 25, 2025 9:03pm

@shleewhite
Copy link
Contributor Author

@hashicorp/hds-engineering should there be a changelog entry for AppHeader and AppSideNav?

@didoo
Copy link
Contributor

didoo commented Apr 22, 2025

@shleewhite a couple of things that jump out to me:

  • we need to re-add them to the showcase demo pages too
  • there were some code in the MockApp for the standalone pages in the showcase that was commented/disabled
  • we need to re-add the percy tests for these pages

maybe you could check the PRs that added (and then removed) all these things, to see what is missing?

@shleewhite
Copy link
Contributor Author

@shleewhite a couple of things that jump out to me:

  • we need to re-add them to the showcase demo pages too
  • there were some code in the MockApp for the standalone pages in the showcase that was commented/disabled
  • we need to re-add the percy tests for these pages

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.

@didoo
Copy link
Contributor

didoo commented Apr 22, 2025

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).

@didoo
Copy link
Contributor

didoo commented Apr 24, 2025

@shleewhite let me know when this is ready for review, I can do it

@didoo
Copy link
Contributor

didoo commented Apr 24, 2025

@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.

didoo
didoo previously approved these changes Apr 24, 2025
Copy link
Contributor

@didoo didoo left a 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>
Copy link
Contributor

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.

Copy link
Contributor Author

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 ¯_(ツ)_/¯

Copy link
Contributor

github-actions bot commented Apr 25, 2025

📦 RC Packages Published

Latest commit: a2bd870

Published 1 packages

@hashicorp/[email protected]

yarn up -C @hashicorp/design-system-components@rc

Copy link
Contributor

@dchyun dchyun left a 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.

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

👍

@shleewhite shleewhite merged commit c2d6888 into main Apr 28, 2025
16 checks passed
@shleewhite shleewhite deleted the hds-4777/revive-enterprise-nav branch April 28, 2025 14:36
@hashibot-hds hashibot-hds mentioned this pull request Apr 28, 2025
@alex-ju alex-ju added this to the [email protected] milestone May 7, 2025
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.

5 participants