-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Customisable Navigation #6111
Customisable Navigation #6111
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/keystonejs/keystone-next-docs/8rjRZZAAWhbWMZYAWDCk9Hh6hdUY |
🦋 Changeset detectedLatest commit: 7be77b7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
20708d0
to
8795064
Compare
ccf4b33
to
374fb5a
Compare
); | ||
}); | ||
})()} | ||
<nav role="navigation" aria-label="Side Navigation" css={{ marginTop: spacing.xlarge }}> |
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.
a11y fix:
- adding a role and aria-label to the side nav for better screen reader support.
- all nav items are now wrapped in an unordered list as per WCAG recommendation.
padding: `${spacing.small}px ${spacing.xlarge}px`, | ||
position: 'relative', | ||
textDecoration: 'none', | ||
<li> |
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.
related to fixes below:
NavItems are now wrapped in a list item for better SR readability and general semantics
Co-authored-by: Tim Leslie <timl@thinkmill.com.au>
Co-authored-by: Tim Leslie <timl@thinkmill.com.au>
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.
yarn lock changes should be reverted
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.
I'm 👍 on this. I'll let @mitchellhamilton have a final review and give it the ✅
export { Logo } from './Logo'; | ||
export { Navigation } from './Navigation'; | ||
export { Navigation, NavigationContainer, NavItem, ListNavItems, ListNavItem } from './Navigation'; |
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.
When would someone want to use Navigation
? Also, it is a different thing to the thing that accepts NavigationProps
, right? (what i would probably expect Navigation
to be is the default of adminConfig.components.Navigation
)
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.
Correct, the wrapping Navigation
component would only be used for custom pages.
For example if someone were to not want to opt into the entire layout provided by PageContainer
but did want to have a sidebar nav, they'd leverage that Navigation export
* add Navigation component to customisable components in adminConfig * add composable navigation components and types to admin-ui/component exports * update to Navigation component * add types for Navigation component * add example for custom-navigation-compoent * add custom route * add example * add guide links * wip nav guide * add minor a11y fixes * update docs and examples * custom admin-ui docs * update yarn.lock * smoke tests for nav example * update test and example * dependency fixes * fix smoke tests * fix failing tests * nope nevermind, not fixed * nav architecture refactor * docs wip * remove d ocs to be added in subsequent PR * type shuffling, so we do not have icky circular type references in @keystone-next/types * changesets * Update .changeset/fluffy-schools-allow.md Co-authored-by: Tim Leslie <timl@thinkmill.com.au> * Update .changeset/rare-carrots-deliver.md Co-authored-by: Tim Leslie <timl@thinkmill.com.au> * yarn.lock reversion * remove unnecessary comments Co-authored-by: Tim Leslie <timl@thinkmill.com.au>
This PR does the following:
AdminConfig
We expose the following components now besides the core Navigation component:
Link
component from Next.jsListMeta
) and renders a NavItem. **ListMeta[]
and an optionalinclude
array. If an include array is specified only list items with keys that match the includes array are rendered as NavItems. Otherwise all lists are rendered as NavItems** The impetus behind
ListNavItem
is to have a higher-level abstraction that can allow users to not have to care about the contents and structure of our lists. This is especially relevant for future feature additions to the nav. (i.e. if we add icon support, that would work out of the box from a dependency bump for users of ListNavItem, whereas you'd have to add that icon prop manually leveraging a lower level component.)**The impetus behind
ListNavItems
is similar, but for rendering a list of NavItems. This is also a pre-emptive DX optimisation for creating custom navigation groups and subsections in the future.Guide and example split into #6185 for expedited functional release.
Preview: