Skip to content
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

AppSideNav - Web docs (HDS-3807) #2410

Open
wants to merge 6 commits into
base: hds-3800-app-sidenav-component
Choose a base branch
from

Conversation

KristinLBradley
Copy link
Contributor

@KristinLBradley KristinLBradley commented Sep 12, 2024

TODO for KB:

  • Remove iconColor API docs

📌 Summary

If merged, this PR will add documentation for the AppSideNav to HDS.

Preview: https://hds-website-git-hds-3807-app-sidenav-docs-hashicorp.vercel.app/components/app-side-nav

Related branches:

🔗 External links


👀 Component checklist

  • [ ] Percy was checked for any visual regression
  • [ ] A changelog entry was added via Changesets if needed (see templates here)

💬 Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Sep 12, 2024

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

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Sep 30, 2024 11:43pm
hds-website ✅ Ready (Inspect) Visit Preview Sep 30, 2024 11:43pm

@hashibot-hds hashibot-hds added the docs-website Content updates to the documentation website label Sep 12, 2024
@KristinLBradley KristinLBradley changed the base branch from main to hds-3800-app-sidenav-component September 16, 2024 16:37
@KristinLBradley KristinLBradley force-pushed the hds-3800-app-sidenav-component branch 3 times, most recently from 024aca1 to 4db2dcf Compare September 20, 2024 18:07
Copy link
Contributor

@shleewhite shleewhite left a comment

Choose a reason for hiding this comment

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

Looks good! Mostly left comments around alt text.

@@ -0,0 +1,5 @@
## 4.10.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this will need to be updated to the correct version number

- `isDesktop`
- `isMinimized`

![Schema of the different visual states in response to the change of viewports and the corresponding logic variables](/assets/components/app-side-nav/responsiveness.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think it would be better to try and get more of this information out of the image and into the page. Maybe a table with the images and captions describing each state?


#### `aria` attributes

The App Side Nav component already provides some of the required `aria` attributes. But other attributes are needed when declaring its content. Please refer to the [Accessibility](/components/app-side-nav?tab=accessibility) section for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: There isn't really anything in the a11y tab describing what attributes are needed when declaring the content. Can you specify here?

@KristinLBradley KristinLBradley changed the title AppSideNav - web docs sub-branch (HDS-3807) AppSideNav - Web docs (HDS-3807) Sep 27, 2024
@jorytindall
Copy link
Contributor

Thanks for all of your detailed suggestions @shleewhite ! I think I caught everything pertaining to the guidelines and the anatomy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-website Content updates to the documentation website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants