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

Jump to bottom

[EuiCollapsibleNav] Fixed docked states on mobile and added more props #3330

Merged
merged 11 commits into from
Apr 16, 2020
Merged

[EuiCollapsibleNav] Fixed docked states on mobile and added more props #3330

merged 11 commits into from
Apr 16, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Apr 15, 2020 •
edited
Loading

Fixes #3302

There were a couple requests addressed in this PR.

1. Fix the onClick handler of the close button when isDocked but in mobile version

The logic here didn't account for this possible state and was updated.

2. Allow the docking breakpoint to be customized

The new dockedBreakpoint allows the consumer to pass a number in pixels to replace the hard-coded internal value (now default) of 992. Eventually this should get updated to be one of our tokens or a global constant, but 🤷 for now.

3. Allow hiding of the floating x close button

I added showCloseButton (with default of true). This thing:
Image 2020-04-15 at 5 29 35 PM

4. [Breaking] Flip the prop name of hideButtonIfDocked to showButtonIfDocked

I didn't catch this before when I was creating the component, but negative props can be difficult to understand, so I flipped this from hide to show. And flipped the default (obviously). At least this was caught pretty early though it is a breaking change.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation 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

cchaos
cchaos commented Apr 15, 2020
View reviewed changes
src/components/collapsible_nav/collapsible_nav.tsx
Comment on lines +80 to +101
window.addEventListener('resize', functionToCallOnWindowResize);

if (navIsDocked) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where the magic happened 😆 🧙

@cchaos cchaos requested a review from chandlerprall April 15, 2020 21:45
@cchaos cchaos force-pushed the fix/collapse_nav_mobile branch from 3e1d46a to ce2240f Compare April 15, 2020 21:55
@cchaos cchaos mentioned this pull request Apr 15, 2020
Closed
@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

chandlerprall
chandlerprall approved these changes Apr 16, 2020
View reviewed changes
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

Would it be feasible to not run the nav-opening animation when toggling from docked to undocked?

open re-animation

@cchaos
Copy link
Contributor Author

cchaos commented Apr 16, 2020

That has been requested before and I'll look into it. But it might be a bit complicated at the moment.

@cchaos
Copy link
Contributor Author

cchaos commented Apr 16, 2020

Ok, turns out it wasn't complicated at all 😆 ... Since the component doesn't re-render when docking, I no longer needed that :not() specification on the selector. So 🎉

Screen Recording 2020-04-16 at 11 41 AM

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor Author

cchaos commented Apr 16, 2020

retest

@cchaos cchaos force-pushed the fix/collapse_nav_mobile branch from 0982f9b to 48f907a Compare April 16, 2020 16:19
@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor Author

cchaos commented Apr 16, 2020

WTF Jenkins.... come on... test this

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

chandlerprall
chandlerprall approved these changes Apr 16, 2020
View reviewed changes
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 still LGTM :)

@cchaos cchaos merged commit 68080d2 into elastic:master Apr 16, 2020
@cchaos cchaos deleted the fix/collapse_nav_mobile branch April 16, 2020 20:45
@myasonik myasonik mentioned this pull request Apr 24, 2020
Closed
@thompsongl thompsongl mentioned this pull request May 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chandlerprall chandlerprall chandlerprall approved these changes

Assignees
No one assigned
Labels
breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EuiCollapsibleNav isDocked state does not work with manual toggle isOpen
3 participants
@cchaos @kibanamachine @chandlerprall