Skip to content

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

Merged
merged 18 commits into from
Apr 28, 2025
Merged

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/design-system-components@4.19.0-rc-20250425201725

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

4 participants