[EuiCollapsibleNavBeta] Add global CSS variable for width offset by cee-chen · Pull Request #7248 · elastic/eui

cee-chen · 2023-10-03T16:48:54Z

Summary

Adding this feature for elastic/kibana#166840 and so Kibana devs don't have to use hard-coded width values for portalled/fixed position content.

QA

Go to https://eui.elastic.co/pr_7248/storybook?path=/story/euicollapsiblenavbeta--global-css-variable
On desktop, toggle the nav between collapsed and non-collapsed and confirm the bottom bar's position updates correctly
Change the viewport to a smaller mobile view and confirm the bottom bar now correctly takes up the full screen width
In devtools' CSS inspector, go to the :root selector and uncheck the --euiCollapsibleNavOffset variable definition - the bottom bar should correctly fall back to a left position of 0

General checklist

Browser QA
- Checked in mobile
- Checked in Chrome, Safari, Edge, and Firefox
  ~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
  ~~- [ ] Checked in both light and dark modes~~
Docs site QA - N/A, beta component
Code quality checklist
- [ ] Added or updated jest and cypress tests Jest tests don't apply here, although a Cypress test could be added - but I'm opting for a Storybook for now, since component is still in beta
Release checklist - N/A, beta component
Designer checklist - N/A

- a unit test can't yet be meaningfully written for this (jsdom doesn't have the concept of `:root` or CSS variables), so this will have to do for now

kibanamachine · 2023-10-03T17:18:13Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7248/
Storybook: https://eui.elastic.co/pr_7248/storybook

elasticmachine · 2023-10-03T17:27:05Z

💚 Build Succeeded

Buildkite Build
Commit: f4638cd

cee-chen · 2023-10-03T17:36:14Z

@nchaulet Feel free to reference this PR when updating yours. Usage should be as simple as <EuiBottomBar left="var(--euiCollapsibleNavOffset, 0)" />.

Check it out: https://eui.elastic.co/pr_7248/storybook/index.html?path=/story/euicollapsiblenavbeta--global-css-variable

breehall

This looks good to me. I've left two very optional questions/items below. I tested in both desktop and mobile view and these work as expected based on your QA criteria.

breehall · 2023-10-03T17:53:35Z

src/components/collapsible_nav_beta/collapsible_nav_beta.tsx

+  useEffect(() => {
+    setGlobalCSSVariables({
+      '--euiCollapsibleNavOffset': isOverlay ? '0' : width,
+    });
+  }, [width, isOverlay, setGlobalCSSVariables]);


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

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

breehall · 2023-10-03T17:57:55Z

src/components/collapsible_nav_beta/collapsible_nav_beta.stories.tsx

 };

+export const GlobalCSSVariable: Story = {
+  render: ({ ...args }) => (


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?

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

breehall · 2023-10-03T17:59:24Z

src/components/collapsible_nav_beta/collapsible_nav_beta.stories.tsx

 };

+export const GlobalCSSVariable: Story = {
+  render: ({ ...args }) => (


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?

@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

Absolutely! That's fine

Sweet! Will update that PR in a second here!

`v88.5.0`⏩`v88.5.4` This EUI upgrade helps unblock the Shared UX team with some beta serverless nav updates not listed in the below changelog (elastic/eui#7228 and elastic/eui#7248). --- ## [`88.5.4`](https://github.com/elastic/eui/tree/v88.5.4) - This release contains internal changes to a beta component needed by Kibana. ## [`88.5.3`](https://github.com/elastic/eui/tree/v88.5.3) **Bug fixes** - Fixed `EuiComboBox` search input width not resetting correctly on selection ([#7240](elastic/eui#7240)) ## [`88.5.2`](https://github.com/elastic/eui/tree/v88.5.2) **Bug fixes** - Fixed broken `EuiTextTruncate` testenv mocks ([#7234](elastic/eui#7234)) ## [`88.5.1`](https://github.com/elastic/eui/tree/v88.5.1) - Improved the performance of `EuiComboBox` by removing the `react-autosizer-input` dependency ([#7215](elastic/eui#7215)) **Dependency updates** - Updated `react-element-to-jsx-string` to v5.0.0 ([#7214](elastic/eui#7214)) - Removed unused `@types/vfile-message` dependency ([#7214](elastic/eui#7214))

cee-chen added 2 commits October 3, 2023 09:43

Add global CSS variable with collapsible nav width

390de42

Add example story

f4638cd

- a unit test can't yet be meaningfully written for this (jsdom doesn't have the concept of `:root` or CSS variables), so this will have to do for now

cee-chen added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Oct 3, 2023

cee-chen marked this pull request as ready for review October 3, 2023 17:33

cee-chen requested a review from a team as a code owner October 3, 2023 17:33

breehall approved these changes Oct 3, 2023

View reviewed changes

cee-chen merged commit 9554450 into elastic:main Oct 3, 2023

cee-chen deleted the collapsible-nav-beta-css-var branch October 3, 2023 18:10

cee-chen mentioned this pull request Oct 3, 2023

Upgrade EUI to v88.5.4 elastic/kibana#167555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

[EuiCollapsibleNavBeta] Add global CSS variable for width offset#7248
cee-chen merged 2 commits intoelastic:mainfrom
cee-chen:collapsible-nav-beta-css-var

cee-chen commented Oct 3, 2023 •

edited

Loading

Uh oh!

kibanamachine commented Oct 3, 2023

Uh oh!

elasticmachine commented Oct 3, 2023

Uh oh!

cee-chen commented Oct 3, 2023

Uh oh!

breehall left a comment

Uh oh!

breehall Oct 3, 2023

Uh oh!

cee-chen Oct 3, 2023

Uh oh!

breehall Oct 3, 2023

Uh oh!

cee-chen Oct 3, 2023

Uh oh!

breehall Oct 3, 2023

Uh oh!

cee-chen Oct 3, 2023

Uh oh!

breehall Oct 3, 2023

Uh oh!

cee-chen Oct 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cee-chen commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

General checklist

Uh oh!

kibanamachine commented Oct 3, 2023

Uh oh!

elasticmachine commented Oct 3, 2023

💚 Build Succeeded

Uh oh!

cee-chen commented Oct 3, 2023

Uh oh!

breehall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cee-chen commented Oct 3, 2023 •

edited

Loading