Skip to content
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

[Serverless Nav] Fix issues with sticky app menu subheader #168372

Merged
merged 18 commits into from
Oct 13, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -203,12 +203,20 @@ export class ChromeService {
};

const headerBanner$ = new BehaviorSubject<ChromeUserBanner | undefined>(undefined);
const bodyClasses$ = combineLatest([headerBanner$, this.isVisible$!, chromeStyle$]).pipe(
map(([headerBanner, isVisible, chromeStyle]) => {
const bodyClasses$ = combineLatest([
headerBanner$,
this.isVisible$!,
chromeStyle$,
application.currentActionMenu$,
]).pipe(
map(([headerBanner, isVisible, chromeStyle, actionMenu]) => {
return [
'kbnBody',
headerBanner ? 'kbnBody--hasHeaderBanner' : 'kbnBody--noHeaderBanner',
isVisible ? 'kbnBody--chromeVisible' : 'kbnBody--chromeHidden',
chromeStyle === 'project' && actionMenu
? 'kbnBody--projectActionMenuVisible'
: 'kbnBody--projectActionMenuHidden',
getKbnVersionClass(),
];
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ interface AppMenuBarProps {
}
export const AppMenuBar = ({ headerActionMenuMounter }: AppMenuBarProps) => {
const { euiTheme } = useEuiTheme();

return (
<div
className="header__actionMenu"
Expand All @@ -28,7 +29,7 @@ export const AppMenuBar = ({ headerActionMenuMounter }: AppMenuBarProps) => {
display: flex;
justify-content: end;
align-items: center;
padding: ${euiTheme.size.s};
height: var(--kbnProjectHeaderAppActionMenuHeight, ${euiTheme.size.xxxl});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the app menu height fixed.

It is also important to not render the menu if there are no actions. There is no good way for app menu component to know if there is content inside. We'd try to improve places where apps set the menu mount point, so that apps stopped setting the mount point if there is no actions

Copy link
Contributor

@cee-chen cee-chen Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels super elegant - it's also great that a plugin could theoretically update the --kbnProjectHeaderAppActionMenuHeight CSS variable and have everything update automatically, if they don't want to use the default xxxl height. Feels really flexible / future-proof!

margin-bottom: -${euiTheme.border.width.thin};
/* fixates the elements position in the viewport, removes the element from the flow of the page */
position: sticky;
Expand Down
4 changes: 4 additions & 0 deletions src/core/public/_css_variables.scss
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
--kbnHeaderOffset: var(--euiFixedHeadersOffset, 0);
// total height of everything when the banner is present
--kbnHeaderOffsetWithBanner: calc(var(--kbnHeaderBannerHeight) + var(--kbnHeaderOffset));
// height of the action menu in the header in serverless projects
--kbnProjectHeaderAppActionMenuHeight: #{$euiSize * 3};
// total height of all fixed headers for the app container in serverless projects, dynamically changes based on whether the banner or action menu is present
--kbnAppHeadersOffset: var(--euiFixedHeadersOffset, 0);
}

// Quick note: This shouldn't be mixed with Sass variable declarations,
Expand Down
6 changes: 3 additions & 3 deletions src/core/public/_mixins.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
@mixin kibanaFullBodyHeight($additionalOffset: 0) {
// The `--euiFixedHeadersOffset` CSS variable is automatically updated by
// The `--kbnAppHeadersOffset` CSS variable is automatically updated by
// styles/rendering/_base.scss, based on whether the Kibana chrome has a
// header banner, and is visible or hidden
height: calc(100vh - var(--euiFixedHeadersOffset, 0) - #{$additionalOffset});
// header banner, app menu, and is visible or hidden
height: calc(100vh - var(--kbnAppHeadersOffset, 0) - #{$additionalOffset});
Copy link
Contributor Author

@Dosant Dosant Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes double scroll in serverless discover caused by incorrect app container height

}
18 changes: 15 additions & 3 deletions src/core/public/styles/rendering/_base.scss
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
pointer-events: none;
visibility: hidden;
position: fixed;
top: var(--euiFixedHeadersOffset, 0);
top: var(--kbnAppHeadersOffset, 0);
right: 0;
bottom: 0;
left: 0;
Expand All @@ -38,12 +38,15 @@
.kbnBody {
padding-top: var(--euiFixedHeadersOffset, 0);
}

// Conditionally override :root CSS fixed header variable. Updating `--euiFixedHeadersOffset`
// on the body will cause all child EUI components to automatically update their offsets

.kbnBody--hasHeaderBanner {
--euiFixedHeadersOffset: var(--kbnHeaderOffsetWithBanner);
--kbnAppHeadersOffset: var(--kbnHeaderOffsetWithBanner);

&.kbnBody--projectActionMenuVisible {
--kbnAppHeadersOffset: calc(var(--kbnHeaderOffsetWithBanner) + var(--kbnProjectHeaderAppActionMenuHeight));
}

// Offset fixed EuiHeaders by the top banner
.euiHeader[data-fixed-header] {
Expand All @@ -56,9 +59,18 @@
top: var(--kbnHeaderBannerHeight);
}
}
.kbnBody--projectActionMenuVisible {
--kbnAppHeadersOffset: calc(var(--kbnHeaderOffset) + var(--kbnProjectHeaderAppActionMenuHeight));
}
.kbnBody--chromeHidden {
--euiFixedHeadersOffset: 0;
&.kbnBody--projectActionMenuVisible {
--kbnAppHeadersOffset: 0;
}
}
.kbnBody--chromeHidden.kbnBody--hasHeaderBanner {
--euiFixedHeadersOffset: var(--kbnHeaderBannerHeight);
&.kbnBody--projectActionMenuVisible {
--kbnAppHeadersOffset: var(--kbnHeaderBannerHeight);
}
}
30 changes: 24 additions & 6 deletions src/plugins/navigation/public/top_nav_menu/top_nav_menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,19 @@ export function TopNavMenu<QT extends AggregateQuery | Query = Query>(
'kbnTopNavMenu__wrapper--hidden': visible === false,
});
if (setMenuMountPoint) {
const badgesEl = renderBadges();
const menuEl = renderMenu(menuClassName);
return (
<>
<MountPointPortal setMountPoint={setMenuMountPoint}>
<span className={`${wrapperClassName} kbnTopNavMenu__badgeWrapper`}>
{renderBadges()}
{renderMenu(menuClassName)}
</span>
</MountPointPortal>
{(badgesEl || menuEl) && (
Copy link
Contributor Author

@Dosant Dosant Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes empty app header for top_nav component, for example, discover doc page:

<UnmountableMountPointPortal setMountPoint={setMenuMountPoint}>
<span className={`${wrapperClassName} kbnTopNavMenu__badgeWrapper`}>
{badgesEl}
{menuEl}
</span>
</UnmountableMountPointPortal>
)}

{renderSearchBar()}
</>
);
Expand All @@ -169,3 +174,16 @@ TopNavMenu.defaultProps = {
showFilterBar: true,
screenTitle: '',
};

// TODO: make this a part of @kbn/react-kibana-mount ?
const UnmountableMountPointPortal: React.FC<{
setMountPoint: (menuMount: MountPoint | undefined) => void;
}> = ({ setMountPoint, children }) => {
React.useEffect(() => {
return () => {
setMountPoint(undefined);
};
}, [setMountPoint]);

return <MountPointPortal setMountPoint={setMountPoint}>{children}</MountPointPortal>;
};
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {
RedirectAppLinks,
useUiSetting$,
} from '@kbn/kibana-react-plugin/public';
import { HeaderMenuPortal } from '@kbn/observability-shared-plugin/public';
import { Router, Routes, Route } from '@kbn/shared-ux-router';
import { euiDarkVars, euiLightVars } from '@kbn/ui-theme';
import React from 'react';
Expand Down Expand Up @@ -183,12 +182,10 @@ export function ObservabilityOnboardingAppRoot({
<i18nCore.Context>
<Router history={history}>
<EuiErrorBoundary>
<HeaderMenuPortal
<ObservabilityOnboardingHeaderActionMenu
setHeaderActionMenu={setHeaderActionMenu}
theme$={theme$}
>
<ObservabilityOnboardingHeaderActionMenu />
</HeaderMenuPortal>
/>
<ObservabilityOnboardingApp />
</EuiErrorBoundary>
</Router>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,43 @@

import { EuiButton } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { HeaderMenuPortal } from '@kbn/observability-shared-plugin/public';
import { AppMountParameters } from '@kbn/core-application-browser';
import { LOGS_ONBOARDING_FEEDBACK_LINK } from '@kbn/observability-shared-plugin/common';
import React from 'react';
import { useLocation } from 'react-router-dom';

export function ObservabilityOnboardingHeaderActionMenu() {
export function ObservabilityOnboardingHeaderActionMenu({
setHeaderActionMenu,
theme$,
}: {
setHeaderActionMenu: AppMountParameters['setHeaderActionMenu'];
theme$: AppMountParameters['theme$'];
}) {
const location = useLocation();
const normalizedPathname = location.pathname.replace(/\/$/, '');

const isRootPage = normalizedPathname === '';

if (!isRootPage) {
return (
<EuiButton
data-test-subj="observabilityOnboardingPageGiveFeedback"
href={LOGS_ONBOARDING_FEEDBACK_LINK}
size="s"
target="_blank"
color="warning"
iconType="editorComment"
<HeaderMenuPortal
Copy link
Contributor Author

@Dosant Dosant Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes empty app header for oblt onboarding page #163326

setHeaderActionMenu={setHeaderActionMenu}
theme$={theme$}
>
{i18n.translate('xpack.observability_onboarding.header.feedback', {
defaultMessage: 'Give feedback',
})}
</EuiButton>
<EuiButton
data-test-subj="observabilityOnboardingPageGiveFeedback"
href={LOGS_ONBOARDING_FEEDBACK_LINK}
size="s"
target="_blank"
color="warning"
iconType="editorComment"
>
{i18n.translate('xpack.observability_onboarding.header.feedback', {
defaultMessage: 'Give feedback',
})}
</EuiButton>
</HeaderMenuPortal>
);
}

Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/observability_onboarding/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"@kbn/share-plugin",
"@kbn/deeplinks-observability",
"@kbn/fleet-plugin",
"@kbn/core-application-browser",
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { useGlobalHeaderPortal } from '../../../../common/hooks/use_global_heade
const StyledStickyWrapper = styled.div`
position: sticky;
z-index: ${(props) => props.theme.eui.euiZHeaderBelowDataGrid};
top: var(--euiFixedHeadersOffset, 0);
top: var(--kbnAppHeadersOffset, 0);
`;

export const GlobalKQLHeader = React.memo(() => {
Expand Down