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

[EuiPageLayout] Add bottomBar & [EuiBottomBar] Add more position props #4662

Merged
merged 31 commits into from
Apr 1, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Mar 24, 2021

This has been a big struggle for some consumers especially when there is side-nav. So this PR builds an optional bottomBar into the page template, while also adding some more props to EuiBottomBar for customizing position.

EuiBottomBar

There had been talk of deprecating the EuiBottomBar for the EuiControlBar, but the lack of custom content in EuiControlBar hasn't allowed us to do that. So now I'm more beefing up the bottom bar that may eventually replace the wrapping component of EuiControlBar which would just be content-specific.

I've also rewritten the component to use useEffect instead of the old way and useResizeObserver() as the size watcher.

New props

  • position: 'static' | 'sticky' | 'fixed' - Defaults to fixed
  • usePortal: boolean - Only works if position is fixed; Defaults to true

🔔 BREAKING: Moved all the positioning attributes to be applied as style attributes.

Any custom classes or CSS used to position the bar, will need to adjust these by add !important for overrides or use the following new props (their defaults shown):

  • left = 0
  • right = 0
  • bottom = 0
  • top

I also darkened the bottom bar color to better match the header.

image

EuiPageTemplate

I found that the easiest way to accomodate side-navs when using the bottom bar, is to place it as the last item inside EuiPageBody with position = sticky. This way it will never overlap the side bar on the left (as it's contained within the page body) and in mobile it will never overlap the revealed navigation list as it will stop being "sticky" when page body is not in view.

Screen.Recording.2021-03-24.at.10.11.26.AM.mp4

So this PR adds the following bottom bar props, but it's only allowed when template = 'default' because empty pages should not have bottom bars when there's nothing to "save".

  • bottomBar: ReactNode - Any contents
  • bottomBarProps: EuiBottomBarProps - Extends the components props

The bottomBar contents also gets wrapped in a EuiPageContentBody so that it can accept the same restrictWidth prop for keeping content aligned.

Image 2021-03-24 at 10 14 38 AM

Other updates

  • Added a bunch of role="main" to the EuiPageContent elements in the template and code examples for accessibility.
  • Added bottomBorder prop to EuiPageHeader, this allows me to enforce the bottom border when I couldn't target it successfully with CSS

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4662/

Comment on lines +140 to +141
// TODO: Allow this hooke to be conditional
const dimensions = useResizeObserver(resizeRef);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't necessarily have to happen in this PR, but we should allow this hook to be conditional somehow. In this particular instance, we don't need to watch for resizing unless the user has specified to affordForDisplacement.

@cchaos cchaos changed the title Feat/bottom bar in page [EuiPageLayout] Add bottomBar & [EuiBottomBar] Add more position props Mar 24, 2021
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4662/

…n_page

# Conflicts:
#	CHANGELOG.md
#	src-docs/src/views/bottom_bar/bottom_bar_example.js
#	src-docs/src/views/bottom_bar/playground.js
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4662/

@cchaos cchaos requested a review from thompsongl March 30, 2021 15:22
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Handful of small asks + one question; substance of changes looks great!

@thompsongl thompsongl removed their request for review March 30, 2021 17:52
@chandlerprall
Copy link
Contributor

PR for you, moving the role=main logic into EuiPageContent: cchaos#47

Moved role=main logic into EuiPageComponent
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4662/

@cchaos
Copy link
Contributor Author

cchaos commented Mar 31, 2021

I need help, because I'm really struggling to understand why this PR would mess up the EuiHeaderSectionItemButton's ref tests:

EuiHeaderSectionItemButton › ref › is the anchor element
    expect(jest.fn()).toHaveBeenCalledTimes(expected)
    Expected number of calls: 1
    Received number of calls: 3

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4662/

@cchaos cchaos requested a review from chandlerprall March 31, 2021 17:36
cchaos added 2 commits March 31, 2021 13:37
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4662/

@cchaos
Copy link
Contributor Author

cchaos commented Mar 31, 2021

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4662/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4662/

cchaos added 2 commits March 31, 2021 19:06
…n_page

# Conflicts:
#	src-docs/src/views/page/page_example.js
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4662/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM, pulled locally to test the template prop interaction with bottomBar/bottomBarProps

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4662/

@chandlerprall chandlerprall merged commit a3eaf22 into elastic:master Apr 1, 2021
@cchaos cchaos deleted the feat/bottom_bar_in_page branch May 11, 2021 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants