-
Notifications
You must be signed in to change notification settings - Fork 42
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
SideNav
component - Porting of advanced features from HcNav
#1304
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
0ab64b9
to
7d7c068
Compare
7d7c068
to
2747ce6
Compare
2747ce6
to
d9fd175
Compare
d9fd175
to
4f4eb84
Compare
4f4eb84
to
cd004de
Compare
cd004de
to
6b2e711
Compare
6b2e711
to
80d8eb8
Compare
80d8eb8
to
bbeb6ac
Compare
bbeb6ac
to
f115c0b
Compare
β¦ container can be used
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.
Impressive work! π
I couldn't spot anything off in my tests so far. Thanks for setting up a preview in Cloud UI, helped a lot to test it in context!
} | ||
|
||
addEventListeners() { | ||
document.addEventListener('keydown', this.escapePress, true); |
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.
not sure if there's a non-convoluted way to add this listener to sidenav's outermost element instead of document
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.
@meirish I think you wrote this code (according to this commit: https://github.com/hashicorp/cloud-ui/commit/f8a3e3a9387a967654742fde2512cae816a1b79d#diff-571b3895fafd8c25954e1f6a914ed1b575c8a7211246259004b521459d5d2e4b)
any ideas/suggestion? specific reasons for attaching the listener to the document ?
|
||
{{#if this.isResponsive}} | ||
{{! template-lint-disable no-invalid-interactive}} | ||
<div class="hds-side-nav__overlay" {{on "click" this.toggleMinimizedStatus}} /> |
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.
Just curious why this needs to be a div
vs. a button
?
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.
no strong reasons, apart from:
- I've ported as is from the existing Cloud UI implementation
- it would be weird to have a huge button covering the entire page (I know it's silly, but the idea makes me nervous)
packages/components/app/styles/components/side-nav/a11y-refocus.scss
Outdated
Show resolved
Hide resolved
packages/components/app/styles/components/side-nav/content.scss
Outdated
Show resolved
Hide resolved
website/docs/components/side-nav/partials/code/component-api.md
Outdated
Show resolved
Hide resolved
website/docs/components/side-nav/partials/code/how-to-use_OLD.md
Outdated
Show resolved
Hide resolved
website/docs/components/side-nav/partials/code/how-to-use_TEMP.md
Outdated
Show resolved
Hide resolved
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.
Looks great to me! I just added a few very minor comments, questions, and suggested edits.
Co-authored-by: Kristin Bradley <kristin.bradley@hashicorp.com>
π Summary
This is a large PR that takes the existing
Hds::SideNav
implementation and extends it to include advanced features like responsiveness (animation/transition) and content "portaling", by adopting (and adapting) the implementation in Cloud UI (then extracted and refactored by @meirish as reusable/standalone component in https://github.com/hashicorp/ember-shared-components).π οΈ Detailed description
Unfortunately, it's not been possible to break down the PR in smaller commits: some of the work has been done in other PRs (see related PRs below) and ported in this PR all together, other work involved large renaming/refactoring of the main component (
SideNav
) as well as its children/sub-components. I'll try to give an overview of what the code changes are, and why they've been done.ember-a11y-refocus
- used to add a navigator narrator to theSideNav
(it's optionally controlled by the consumers)ember-stargate
- used for content "portaling" (see the Cloud UI implementation to understand how this worksSideNav
component porting advanced features fromHcNav
(responsiveness/animation/portaling)SideNav
SideNav::Wrapper
container, with the high-level functionalities provided by theHcNav
implementation:a11y-refocus
<NavigatorNarrator>
elementhasA11yRefocus
argumenttrue
by default)header/body/footer
containers of the "sidenav" as named blocks and exposed theisMinimized
tracked property so it can be used for custom behaviours if necessary--hds-app-desktop-breakpoint
)esc
is added (if triggered it minimizes the sidenav)onDesktopViewportChange
andonToggleMinimizedStatus
so consumers can hook on these if they needhds-side-nav-hide-when-minimized
class name is provided so consumers can use it with custom element, to control how they respond to changes in the "minimised" stateSideNav::Wrapper
SideNav
top-level component (with/without responsiveness)SideNav::Header
SideNav::HomeLink
andSideNav::IconButton
under the "header" folder for better file organizationSideNav::List
list to account for potential future use cases
SideNav::PortalTarget
+SideNav::Portal
SideNav
component and sub-componentsπ Previews
π§ Todo
add JSON design tokens for the CSS variablesupdate the "how to use" section of the documentationstandardizez-index
values and expose them as design tokens (JSON + CSS variables)π External links
Jira tickets:
Related PRs:
SideNav
component (HDS-1143)Β #1163AppFrame
componentΒ #1284π Reviewer's checklist:
π¬ Please consider using conventional comments when reviewing this PR.