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.

Sign up for GitHub

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

[EuiCollapsibleNavBeta] Add global CSS variable for width offset #7248

Merged
merged 2 commits into from
Oct 3, 2023
Merged

[EuiCollapsibleNavBeta] Add global CSS variable for width offset #7248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
390de42
Add global CSS variable with collapsible nav width
cee-chen Oct 3, 2023
f4638cd
Add example story
cee-chen Oct 3, 2023
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
19 changes: 19 additions & 0 deletions src/components/collapsible_nav_beta/collapsible_nav_beta.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import type { Meta, StoryObj } from '@storybook/react';

import { EuiHeader, EuiHeaderSection, EuiHeaderSectionItem } from '../header';
import { EuiPageTemplate } from '../page_template';
import { EuiBottomBar } from '../bottom_bar';
import { EuiFlyout } from '../flyout';
import { EuiButton } from '../button';
import { EuiTitle } from '../title';
Expand Down Expand Up @@ -469,6 +470,24 @@ export const FlyoutInFixedHeaders: Story = {
},
};

export const GlobalCSSVariable: Story = {
render: ({ ...args }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is args being set to anything here? Is it just the component defaults? If not, could we get away without passing anything to the render function?

Copy link
Member Author

Choose a reason for hiding this comment

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

We always need to pass ...args (if there's a control panel), as that's where the control values come from

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about hiding initialIsCollapsed and onCollapseToggle from this story since it's mainly to show how the bottom bar is affected by the width of the nav?

Copy link
Member Author

Choose a reason for hiding this comment

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

@breehall Would you be cool with me making that change in #7245 as opposed to this PR? I'd like to do a release with this work so that I can get it into the currently open EUI upgrade and @nchaulet can update his Kibana PR sooner rather than later.

There's a bunch of other stories under this file that need a closer inspection of their props pane in any case, and I'd rather have the new util I just set up while doing so

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely! That's fine

Copy link
Member Author

Choose a reason for hiding this comment

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

Sweet! Will update that PR in a second here!

<>
<EuiHeader position="fixed">
<EuiHeaderSection>
<EuiCollapsibleNavBeta {...args}>
This story tests the global `--euiCollapsibleNavOffset` CSS variable
</EuiCollapsibleNavBeta>
</EuiHeaderSection>
</EuiHeader>
<EuiBottomBar left="var(--euiCollapsibleNavOffset, 0)">
This text should be visible at all times and the bar position should
update dynamically based on the nav width (including on mobile)
</EuiBottomBar>
</>
),
};

export const CollapsedStateInLocalStorage: Story = {
render: () => {
const key = 'EuiCollapsibleNav__isCollapsed';
Expand Down
18 changes: 16 additions & 2 deletions src/components/collapsible_nav_beta/collapsible_nav_beta.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@ import React, {
} from 'react';
import classNames from 'classnames';

import { useEuiTheme, useGeneratedHtmlId, throttle } from '../../services';
import {
useEuiTheme,
useEuiThemeCSSVariables,
useGeneratedHtmlId,
throttle,
} from '../../services';

import { CommonProps } from '../common';
import { EuiFlyout, EuiFlyoutProps } from '../flyout';
Expand Down Expand Up @@ -88,6 +93,7 @@ const _EuiCollapsibleNavBeta: FunctionComponent<EuiCollapsibleNavBetaProps> = ({
focusTrapProps: _focusTrapProps,
...rest
}) => {
const { setGlobalCSSVariables } = useEuiThemeCSSVariables();
const euiTheme = useEuiTheme();
const headerHeight = euiHeaderVariables(euiTheme).height;

Expand Down Expand Up @@ -138,9 +144,17 @@ const _EuiCollapsibleNavBeta: FunctionComponent<EuiCollapsibleNavBetaProps> = ({
const width = useMemo(() => {
if (isOverlayFullWidth) return '100%';
if (isPush && isCollapsed) return headerHeight;
return _width;
return `${_width}px`;
}, [_width, isOverlayFullWidth, isPush, isCollapsed, headerHeight]);

// Other UI elements may need to account for the nav width -
// set a global CSS variable that they can use
useEffect(() => {
setGlobalCSSVariables({
'--euiCollapsibleNavOffset': isOverlay ? '0' : width,
});
}, [width, isOverlay, setGlobalCSSVariables]);
Comment on lines +152 to +156
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very clever fix. I actually didn't know we had a styling function to globally set variables.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was added in #7151 / #7132! It's always nice when some architecture you set up pays off almost immediately and Just Works ✨


/**
* Prop setup
*/
Expand Down
Loading