[Workspace Chrome] Bootstrap grid layout for classic nav#224255
[Workspace Chrome] Bootstrap grid layout for classic nav#224255Dosant merged 9 commits intoelastic:mainfrom
Conversation
71abd68 to
a4101de
Compare
| const badge$ = new BehaviorSubject<ChromeBadge | undefined>(undefined); | ||
| const customNavLink$ = new BehaviorSubject<ChromeNavLink | undefined>(undefined); | ||
| const helpSupportUrl$ = new BehaviorSubject<string>(docLinks.links.kibana.askElastic); | ||
| const isNavDrawerLocked$ = new BehaviorSubject(localStorage.getItem(IS_LOCKED_KEY) === 'true'); |
There was a problem hiding this comment.
This is not used; it's just a cleanup.
| recentlyAccessed, | ||
| docTitle, | ||
| getHeaderComponent, | ||
| getLegacyHeaderComponentForFixedLayout, |
There was a problem hiding this comment.
- getLegacyHeaderComponentForFixedLayout (getHeaderComponent) - is mostly untouched and still responsible for rendering the header in the old layout which is fixed, includes a banner, swaps between classic and project nav, handles chromeless state
- getClassicHeaderComponentForGridLayout - header to use in grid layout for classic navigation, doesn't include banner and chromeless state that is now is handled by the layout service
- getHeaderBanner, getChromelessHeader - used by the new layout
| } | ||
|
|
||
| const styles = { | ||
| breadcrumbsWithExtensionContainer: css` |
There was a problem hiding this comment.
Simplifying global chrome styles by moving what is possible to the components and emotion
|
|
||
| return ( | ||
| <div className="chrHeaderBadge__wrapper"> | ||
| <div css={({ euiTheme }) => ({ alignSelf: 'center', marginLeft: euiTheme.size.base })}> |
There was a problem hiding this comment.
Simplifying global chrome styles by moving what is possible to the components and emotion
| <EuiFlexItem | ||
| grow={false} | ||
| className="chrHeaderHelpMenu__version" | ||
| css={{ textTransform: 'none' }} |
There was a problem hiding this comment.
Simplifying global chrome styles by moving what is possible to the components and emotion
|
|
||
| // Due to pure HTML and the scope being large, we decided to temporarily apply following 3 style blocks globally. | ||
| // TODO: refactor within github issue #223571 | ||
| const hackGlobalFieldFormattersPluginStyles = (euiTheme: UseEuiTheme['euiTheme']) => css` |
There was a problem hiding this comment.
just moving to a more appropriate place to share between both layouts
| import { CommonGlobalAppStyles } from '../common/global_app_styles'; | ||
| import { useSyncPushFlyoutHack, euiPushFlyoutPaddingInlineEnd } from './hack_use_sync_push_flyout'; | ||
|
|
||
| const globalLayoutStyles = (euiTheme: UseEuiTheme['euiTheme']) => css` |
There was a problem hiding this comment.
we consolidate and split important global styles for each layout. grid and legacy-fixed layouts have their own copy and are different.
| `; | ||
|
|
||
| // temporary hacks that need to be removed after better flyout and global sidenav customization support in EUI | ||
| const globalTempHackStyles = (euiTheme: UseEuiTheme['euiTheme']) => css` |
There was a problem hiding this comment.
global css overrides hacks for eui are consolidated here. the goal is to merge as is, but to get rid of it on EUI side before this goes to prod. elastic/eui#8820
There was a problem hiding this comment.
nit: add link to eui issue.
| * Currently, EUI push flyouts visually push the content to the right by adding a padding to the body | ||
| * This hook listens to styles changes on the body and updates a CSS variable that is used to push the workspace content | ||
| */ | ||
| export function useSyncPushFlyoutHack() { |
There was a problem hiding this comment.
dirty hack to position push flyout inside the the application container. the goal is to merge as is, but to get rid of it on EUI side before this goes to prod. elastic/eui#8820
There was a problem hiding this comment.
nit: we might consider naming things like this useHackSyncPushFlyout rather than useSyncPushFlyoutHack.
If there wasn't a lint rule around Rules of Hooks™️ I would name it hack__useSyncPushFlyout. 🤷🏻
And please add a link to the issue in the comment.
There was a problem hiding this comment.
I also wanted to start with hack...
| const sideBarProps = { ...pageSideBarProps }; | ||
| sideBarProps.sticky = true; | ||
| // TODO: instead of using sticky = true here, we reproduce the same behavior to account for both legacy fixed layout and new grid layout. | ||
| sideBarProps.style = { |
There was a problem hiding this comment.
another temp hack for eui: how sticky: true works doesn't work properly with new layout, so I had to customize styles here so that it works for both fixed and grid kibana layout elastic/eui#8820
There was a problem hiding this comment.
nit: link to eui issue in the comment.
|
Pinging @elastic/appex-sharedux (Team:SharedUX) |
clintandrewhall
left a comment
There was a problem hiding this comment.
This is impressive work, Anton. Really great, and fast!
I'm approving to unblock, but please address the nits. My biggest is eliminating useObservable by subscribing to the Observables in the Layout services, passing their values as props. It's a subtle (but proven important) optimization.
| return <HeaderComponent />; | ||
| }; | ||
|
|
||
| const getClassicHeaderComponentForGridLayout = () => { |
There was a problem hiding this comment.
nit: ChromeService has been driving me crazy with all the methods it contains, apparently scoped to different use cases.
It may not be germane to this PR, but we've got to find some time to clean this thing up... or at the very least, split it up somehow. Perhaps make this an abstract class with use-case implementations?
Again: I think something for later. But I'm resigned to see another pair of use-case specific, complex-named methods added to this service.
There was a problem hiding this comment.
Totally agree! I think now with a feature flag, would make sense to rollout the grid layout -> clean up the old layout -> then do a proper refactor of chrome service because code and testing scope would be smaller. I captured an issue #225264 and will leave it in the code to make it clearer
| /> | ||
|
|
||
| <EuiHeader position="fixed" className="header__secondBar"> | ||
| <EuiHeader position={isFixed ? 'fixed' : 'static'} className="header__secondBar"> |
There was a problem hiding this comment.
Does this classname still apply?
src/core/packages/chrome/browser-internal/src/ui/header/header_top_banner.tsx
Outdated
Show resolved
Hide resolved
src/core/packages/chrome/browser-internal/src/ui/header/header_top_banner.tsx
Outdated
Show resolved
Hide resolved
src/core/packages/chrome/browser-internal/src/ui/header/header_top_banner.tsx
Outdated
Show resolved
Hide resolved
| * Currently, EUI push flyouts visually push the content to the right by adding a padding to the body | ||
| * This hook listens to styles changes on the body and updates a CSS variable that is used to push the workspace content | ||
| */ | ||
| export function useSyncPushFlyoutHack() { |
There was a problem hiding this comment.
nit: we might consider naming things like this useHackSyncPushFlyout rather than useSyncPushFlyoutHack.
If there wasn't a lint rule around Rules of Hooks™️ I would name it hack__useSyncPushFlyout. 🤷🏻
And please add a link to the issue in the comment.
|
|
||
| return React.memo(() => { | ||
| // TODO: optimize initial value to avoid unnecessary re-renders | ||
| const chromeVisible = useObservable(chromeVisible$, false); |
There was a problem hiding this comment.
Get rid of the Observable and this gets easy.
| const { chrome, featureFlags } = renderCoreDeps; | ||
| const layoutType = featureFlags.getStringValue<LayoutFeatureFlag>( | ||
| LAYOUT_FEATURE_FLAG_KEY, | ||
| 'legacy-fixed' |
There was a problem hiding this comment.
nit: consider a constant? Or is this strongly-typed?
| const sideBarProps = { ...pageSideBarProps }; | ||
| sideBarProps.sticky = true; | ||
| // TODO: instead of using sticky = true here, we reproduce the same behavior to account for both legacy fixed layout and new grid layout. | ||
| sideBarProps.style = { |
There was a problem hiding this comment.
nit: link to eui issue in the comment.
| `; | ||
|
|
||
| // temporary hacks that need to be removed after better flyout and global sidenav customization support in EUI | ||
| const globalTempHackStyles = (euiTheme: UseEuiTheme['euiTheme']) => css` |
There was a problem hiding this comment.
nit: add link to eui issue.
gsoldevila
left a comment
There was a problem hiding this comment.
Core changes LGTM (code review only)
ek-so
left a comment
There was a problem hiding this comment.
lgtm (I checked visually, don't see the difference)
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Count of Enzyme imports
Page load bundle
Unknown metric groupsAPI count
History
|
> [!IMPORTANT] > **Should be no user-facing changes!!!** The new layout work is behind a feature flag! ## Summary Part of [workspace chrome](elastic/kibana-team#1581 ) work. This is follow up to #224255, #225824 which mostly implements the new layout for project (serverless) navigation.  ### Implementation notes: - The project navigation sidebar still uses the EuiCollapsibleNavBeta component. It works as a push flyout on larger screens and as an overlay on smaller screens. - In the next steps, we will start using the new navigation grid cell for the upcoming side navigation component. ## How to review 1. Most importantly, we need to ensure that nothing is broken in the old layout during the refactor. - Functional tests + visual/manual testing 2. Then for the new layout: kibana.yml: ``` feature_flags.overrides: core.chrome.layoutType: 'grid' core.chrome.layoutDebug: true ``` - Check that it mostly works (some specific edge cases and bugs are expected and are gathered in #225263) - Code-review: focus on the layout implementation split approach
) > [!IMPORTANT] > **Should be no user-facing changes!!!** The new layout work is behind a feature flag! ## Summary Part of [workspace chrome](elastic/kibana-team#1581 ) work. This is follow up to elastic#224255, elastic#225824 which mostly implements the new layout for project (serverless) navigation.  ### Implementation notes: - The project navigation sidebar still uses the EuiCollapsibleNavBeta component. It works as a push flyout on larger screens and as an overlay on smaller screens. - In the next steps, we will start using the new navigation grid cell for the upcoming side navigation component. ## How to review 1. Most importantly, we need to ensure that nothing is broken in the old layout during the refactor. - Functional tests + visual/manual testing 2. Then for the new layout: kibana.yml: ``` feature_flags.overrides: core.chrome.layoutType: 'grid' core.chrome.layoutDebug: true ``` - Check that it mostly works (some specific edge cases and bugs are expected and are gathered in elastic#225263) - Code-review: focus on the layout implementation split approach
Important
Should be no user-facing changes!!! The new layout work is behind a feature flag!
Important
This bootstraps new grid layout for chrome using a feature flag. It only works with classic nav and hack a lot of bugs and EUI-related workarounds, but the overall code structure and approach can be reviewed and merged to main.
Summary
Part of workspace chrome work. In this PR we lay down the ground work for new grid layout that will power Kibana's chrome. This is done by introducing a feature flag with which Kibana can switch between "legacy-fixed" layout and new "grid" layout.
Proper detailed figma link: https://www.figma.com/design/10ca4AhnWDkyJklUDXnHg5/Sidebar?node-id=5192-259808&p=f&m=dev
kibana.yml:
For this, in-between
rendering_serviceandchrome_servicea newlayout_servicewas introduced the goal of which is to aggregate stuff from chrome service and compose it together using the needed layout. There are two implementations forlayout_service:LegacyFixedLayout- old one, just code refactor, should still work as in mainGridLayout- new one, mostly works, but only for classic nav, for now, and with bunch of hacks and bugs that we will resolve over timeThe switch is in
rendering_servicebased on a feature flag:To see the grid and new layout in action there is a helpful
debugflag that displays not yet used elements of new layout:kibana.yml:
demo-grid.mov
Other clean ups
.chrHeaderBadge__wrapper,. chrHeaderHelpMenu__version,breadcrumbsWithExtensionContainerto emotion on simplify global css of chromegetIsNavDrawerLockedand related css since not usedTODO
How to review
kibana.yml: