resolve issue 39 -smart bulletin and added nav structure for policies and audits section #188
Conversation
weiwang-gsa
commented
Jul 12, 2023
•
weiwang-gsa
commented
- added navigation structure for policies and audits
- added smart bulletins
- resolve link button issue in individual audits
change side nave to use yaml data too
Fix audit page path typos
Fixes document path
… into wwang-issue39
weiwang-gsa
changed the title
src/components/SideNav.astro
Outdated
| @@ -1,7 +1,6 @@ | |||
| --- | |||
| const { id, pages, headings, parent_path, title} = Astro.props; | |||
| const { id, pages, headings, parent_path, title, additional_menu=null} = Astro.props; | |||
There was a problem hiding this comment.
It doesn't seem like the additional_menu props is used in the component. Can we remove it?
There was a problem hiding this comment.
definitely, will clean it up
src/components/TopNav.astro
Outdated
|
|
||
| --- | ||
| <div class="display-flex margin-bottom-2" style="width:100%"> | ||
| <img src={previousNavIcon} alt="left Arrow icon for navigation back to Policies and Audits"><a class="usa-link" href={`${import.meta.env.BASE_URL}${parent_nav_path}`}>Return to the {parent_nav_text}</a> |
There was a problem hiding this comment.
We can talk about how these links will work when Kristen is back, but in general we don't want screen readers to see the icons. The code in https://designsystem.digital.gov/components/pagination/ is a good example — they implement the arrow icon with aria-hidden="true" role="img" to prevent screen readers from see the icon.
There was a problem hiding this comment.
sure, per our discussion, I keep current top nav using link (matching mockup) for now, once we talked to Kirsten, we can update the TopNav based on final decision, thanks!
|
|
||
| <PageLayout title ={entry.data.title} > | ||
|
|
||
| <TopNav parent_nav_path="policies-and-audits/audits" parent_nav_text = "Audit Repository" /> |
There was a problem hiding this comment.
I wonder if the TopNav should be part of the menu component. There are two reasons:
- It is semantically part of the navigation so one could argue it belongs inside the
<nav></nav>If we want to change this, we will probably need to alter theSideNavcomponent to accept an optional text/link. - In the prototype this link is vertically aligned with the content's main heading. When this component is in its own div, it pushed the main content down so it is no longer at the top of the page.
There was a problem hiding this comment.
Thank you! Moved TopNav into Sidenav. code checked in
| } | ||
| }) | ||
| } | ||
| const { entry, entries} = Astro.props; |
There was a problem hiding this comment.
I don't think we use entries in the component. Can we remove it?
There was a problem hiding this comment.
sure, will remove it
