-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Chrome] Prep for new side navigation #229110
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
[Chrome] Prep for new side navigation #229110
Conversation
|
Pinging @elastic/appex-sharedux (Team:SharedUX) |
| toggleSideNav={setIsSideNavCollapsed} | ||
| > | ||
| {includeSideNavigation ? getProjectSideNavComponent() : null} | ||
| <Router history={application.history}> |
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.
Note: moved the Router rendering up from https://github.com/elastic/kibana/pull/229110/files#diff-333fe32fe36772f661ce2091cb24ec608f14efda629e44abd1ada1ae8d8ddb0aL261
| /** | ||
| * Used only by the rendering service to render the new project side navigation UI | ||
| * | ||
| * @deprecated - clean up https://github.com/elastic/kibana/issues/225264 |
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.
Nit:
| * @deprecated - clean up https://github.com/elastic/kibana/issues/225264 | |
| * @deprecated - do not extend this, needs cleanup https://github.com/elastic/kibana/issues/225264 |
tsullivan
left a comment
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.
Approving to unblock.
The relocations make sense from a code organization perspective.
The new semantics make sense as well.
The TODOs and commented code is acceptable, as this PR is part of a set of isolated PRs that are closely follow each other.
src/core/packages/chrome/browser-internal/src/ui/project/sidenav_v2/collapse_button.tsx
Outdated
Show resolved
Hide resolved
src/core/packages/chrome/browser-internal/src/ui/project/sidenav_v2/collapse_button.tsx
Outdated
Show resolved
Hide resolved
src/core/packages/chrome/browser-internal/src/ui/project/sidenav_v2/collapse_button.tsx
Outdated
Show resolved
Hide resolved
src/core/packages/chrome/browser-internal/src/ui/project/sidenav_v2/collapse_button.tsx
Show resolved
Hide resolved
src/core/packages/chrome/browser-internal/src/ui/project/sidenav_v2/collapse_button.tsx
Outdated
Show resolved
Hide resolved
src/core/packages/chrome/browser-internal/src/ui/project/sidenav_v2/collapse_button.tsx
Show resolved
Hide resolved
src/core/packages/chrome/browser-internal/src/ui/project/sidenav_v2/fixed_layout_sidenav.tsx
Show resolved
Hide resolved
src/core/packages/chrome/browser-internal/src/ui/project/sidenav_v2/fixed_layout_sidenav.tsx
Outdated
Show resolved
Hide resolved
| // items={demoItems} | ||
| // logoLabel={LOGO.label} | ||
| // logoType={LOGO.logoType} | ||
| // setWidth={setWidth} |
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.
non-blocking, just thinking out-loud:
Like we talked, setWidth feels awkward and like it doesn't belong here. The reason we are using it is because the side nav changes its width: expanded, collapsed, side panel open, side panel closed. The new grid layout needs to know in advance the static value for the side navigation width (and all other cells as well). There are 2 variables here: isCollapsed and isSidePanelOpen.
I thought about this and I believe passing through a memoized setter is probably the simplest solution.
What we could do is create a hook that does this width calculation, expose it through @kbn/core-chrome-navigation, and then feed isCollapsed and isSidePanelOpen from the parent into the navigation component but this would result in even more props passed through and consequently potential wasted re-renders on opening the side panel. We could also feed this data through context inside the navigation component but that would couple it with the usage.
Personally, I'd proceed with setWidth prop.
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.
setWidth seem to be working fine for now for both grid and fixed, so I agree to proceed like this and then review if needed 👍
Only, i'd maybe rename this prop to onWidthChange or onResize or something like that?
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.
It would imply that there's some change happening in the navigation component that the parent might want to listen into. But the fact is, we're passing a setter and we're using it in the navigation component for the parent's sake, right?
At the same time, onWidthChange sounds more natural. I don't have anything against using that.
But onResize might not be a good idea considering we want the side panel to be resizable in the future.
…igation-components-old-layout
src/core/packages/chrome/browser-internal/src/ui/project/sidenav_v2/fixed_layout_sidenav.tsx
Show resolved
Hide resolved
💔 Build Failed
Failed CI Steps
Metrics [docs]Module Count
Public APIs missing comments
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
|
Summary
Most of it is extracted from #228162 plus some changes to support old layout
This refactor establishes a foundation for new side navigation integration while maintaining backward compatibility through the v1/v2 component structure and feature flagging system.
chrome_service.tsxNavigation Rendering Strategy
ProjectNavigationrendering fromProjectHeadertochrome_servicefor easier v1/v2 swapping.Simplified Header Semantics
asprop on header component (now uses semantic<header>).<div>to prevent duplicate header elements.Streamlined API Naming
isSideNavCollapsed$→isCollapsed$toggleSideNav→sidenavNew Side Navigation Components Entry Points
SideNavV2CollapseButton: Custom collapse button based onEuiCollapsibleNavBeta, tailored for new sidenav interaction patterns.fixed_layout_sidenav.tsx: Entry point for new sidenav with legacy fixed layout system.grid_layout_sidenav.tsx: Entry point for new sidenav with modern grid layout system.sidenav_v1/navigation.tsxfor backward compatibility.core-chrome-layoutComponent Cleanup
navigationPanel: No longer needed in new architecture.<div>instead of semantic<nav>for layout flexibility.Layout Improvements
<div>as chrome layer now handles semantics.max-widthto header for proper breadcrumb truncation.useLayoutUpdatefor responsive layout changes when nav is toggled.