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

Upgrade EUI to 13.6.0 #43916

Merged
merged 12 commits into from
Aug 27, 2019
Merged

Upgrade EUI to 13.6.0 #43916

merged 12 commits into from
Aug 27, 2019

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Aug 23, 2019

eui@13.3.0eui@13.6.0

Enhancement: The side nav bar can now be locked open. Space is adjusted in the main page area to allow all content to be seen (not hidden behind the nav). The user's choice to lock/unlock is stored in localStorage and will be honored on subsequent view changes and page reloads.
Fixes #33328


13.6.0

  • Revert conversion of EuiSwitch to button[role=switch] and TypeScript (#2255)

13.5.0

  • Fixed logoCloudEnterprise, logoLogging, and logoSecurity SVGs in EuiIcon to be center aligned (#2246)
  • Added locking behavior of EuiNavDrawer expanded state inluding the following props isLocked, onIsLockedUpdate (#2247)

13.4.1

  • Converted EuiSwitch to TypeScript (#2243)

Bug fixes

  • Added missing viewBox attribute to Docker, Kubernetes, and Redis logos (#2240)

13.4.0

  • Converted EuiFacetButton to TypeScript (#2226)
  • Added an optional onClear prop to the the EuiDatePicker component (#2235)
  • Added support for onClick and href props on EuiListGroupItem and converted to TypeScript (#1933)

Bug fixes

  • Fixed EuiSwitch semantics to align with aria roles (#2193)
  • Removed Firefox's focus ring to match other browsers (#2193)
  • Added missing onChange TS defs for EuiRange (#2211)
  • Fixed EuiBadge text cursor to default pointer (#2234)
  • Fixed EuiPageContent className prop to allow the passed-in className to take cascade precedence over classes generated by the component (#2237)

@thompsongl thompsongl added chore v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.4.0 labels Aug 23, 2019
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Looks like the nav changes are safe. We'll want to change them to incorporate the new functionality, but it looks like as it right now they still work as they did previously. That means we can do the added stuff as a separate PR if this goes green and we want to merge it. Might want to triple check with @cchaos there though.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

This will require some changes for the nav. We'll take care of it on monday.

@cchaos
Copy link
Contributor

cchaos commented Aug 26, 2019

@thompsongl I could use your help here since we will actually need that onIsLockedUpdate function to traverse two levels up somehow. Essentially, looking at the DOM we need:

@cchaos
Copy link
Contributor

cchaos commented Aug 26, 2019

Actually looking at it some more, it's possible we would just need the new class at the .header-global-wrapper level.

@joshdover
Copy link
Contributor

It's worth mentioning that the hierarchy of these tags at the header / app-wrapper level are kind of a mess. I've been hitting up against this with moving some global UI to the New Platform.

I'd really like to clean up the hierarchy here and its associated CSS, but we've been afraid of breaking selectors in subtle ways.

That said, I think the solution of adding a class to the header-global-wrapper should "work" but only if you use a sibling selector: .header-global-wrapper.nav-drawer-locked + .app-wrapper. This is sorta limiting since it means we can't have any other tags between those components.

@cchaos
Copy link
Contributor

cchaos commented Aug 26, 2019

@joshdover That was what I was thinking, but you can use the indirect sibling selector ~ so that it doesn't have to be directly next to, but at the same level of the .header-global-wrapper.

@thompsongl thompsongl requested a review from a team as a code owner August 26, 2019 21:18
@thompsongl
Copy link
Contributor Author

thompsongl commented Aug 26, 2019

@cchaos The class name toggle is in place. I would imagine someone will have RxJS suggestions for me, but styling can be done at this point.

@joshdover
Copy link
Contributor

@joshdover That was what I was thinking, but you can use the indirect sibling selector ~ so that it doesn't have to be directly next to, but at the same level of the .header-global-wrapper.

Ah right I forgot that selector existed. That should work fine for now then.

@thompsongl
Copy link
Contributor Author

@joshdover Wrapper component added, no longer using an Observable for locked state.

cchaos
cchaos previously requested changes Aug 26, 2019
src/core/public/chrome/ui/header.tsx Outdated Show resolved Hide resolved
And updated BEM naming of `header-global-wrapper`
@cchaos
Copy link
Contributor

cchaos commented Aug 26, 2019

@thompsongl PR4U: thompsongl#2

@cchaos cchaos dismissed their stale review August 26, 2019 22:38

Made the changes during my CSS additions

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Just a quick question about the storage of the setting.

src/core/public/chrome/ui/header/header_wrapper.tsx Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor

cchaos commented Aug 27, 2019

@snide I noticed that too and also think that the only fix for it is to store the user preference somehow. However, I think that shouldn't hold up this PR from actually getting merged. But we should definitely make sure that functionality makes it into 7.4.

@spalger had created something a while back but it never got used. Maybe he can point us to the right files so we can get it hooked up.

@joshdover
Copy link
Contributor

I don't think we need full-on user preferences that are stored in ES or anything. It seems to me localStorage would be fine?

@snide
Copy link
Contributor

snide commented Aug 27, 2019 via email

@thompsongl
Copy link
Contributor Author

localStorage now in use

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM. I found an issue where the title text for the icon was reversed, but have a PR up on EUI for this fix already. elastic/eui#2261

@cchaos
Copy link
Contributor

cchaos commented Aug 27, 2019

We should probably mark this with release-note: enhancement and a note to the docs writers about the nav toggle

@thompsongl thompsongl added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Aug 27, 2019
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Thanks folks. Much better!

@thompsongl thompsongl requested a review from joshdover August 27, 2019 15:39
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor

cchaos commented Aug 27, 2019

jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@thompsongl thompsongl merged commit 084433f into elastic:master Aug 27, 2019
thompsongl added a commit to thompsongl/kibana that referenced this pull request Aug 27, 2019
* eui to 13.6.0

* euirange updates

* euipage snapshot updates

* add classname toggle for nav locking

* new header wrapper component; removed observable

* Add styles for locked nav

And updated BEM naming of `header-global-wrapper`

* move headerwrapper

* isLocked localStorage

* remove useEffect
thompsongl added a commit that referenced this pull request Aug 27, 2019
* eui to 13.6.0

* euirange updates

* euipage snapshot updates

* add classname toggle for nav locking

* new header wrapper component; removed observable

* Add styles for locked nav

And updated BEM naming of `header-global-wrapper`

* move headerwrapper

* isLocked localStorage

* remove useEffect
@fbaligand
Copy link
Contributor

I love the “lock side bar” button!
Thanks for this feature!

It solves my issue: #43019

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.

Nav drawer UX
6 participants