[7467] Add "Collapse/Expand" button to sidebar. #7723

cjcenizal · 2016-07-12T17:13:59Z

Addresses #7467.

Blocked by #7711. Once that PR is merged, we'll rebase this one.

Changes

Allow user to pin/unpin the sidebar, and store preference in localStorage.
Add "Expand/Collapse" button to sidebar.
Add appSwitcherState service.
Add support for "classes" attribute to app-switcher-link.
Remove animation from sidebar, and no longer expand on hover.
Add default tooltips for sidebar items.

Expanded and collapsed states

Note: If there is a tooltip specified for an app, then the app's title will be prepended to the tooltip text, e.g. "Visualize - Make your data beautiful."

Tooltips on disabled links

kimjoar · 2016-07-12T17:22:22Z

Can you include a screenshot or gif in PRs such as this? Makes it a lot easier for us that don't have Kibana running all the time to see how it looks like (and to review the non-code parts of the PR)

(I recommend Gif Brewery if you want to make gifs, it's awesome)

cjcenizal · 2016-07-12T17:49:00Z

@kjbekkelund Great call on Gif Brewery.

alt74 · 2016-07-12T18:55:21Z

very nice and simple. I dig it. thanks @cjcenizal 👍

Bargs · 2016-07-14T21:39:17Z

You added the tooltips ❤️ ❤️ ❤️ ❤️

kimjoar · 2016-08-01T16:10:07Z

src/ui/public/chrome/directives/app_switcher_link/__tests__/app-switcher-link.js

+          appSwitcherLinkClasses: 'new classes',
+        };
+        const element = create(attrs);
+        expect(element.attr('class')).to.contain(attrs.appSwitcherLinkClasses);


I've never been a big fan of tests that use the same data as input and as what they check. What if create(attrs) didn't work so class is wrong, but because you have a typo in attrs.appSwitcherLinkClassses (aka it's undefined) it doesn't fail? I much prefer actually typing it out.

I think that's a very reasonable point. The reason I like this pattern is that it makes intention crystal-clear; there's no doubt what I want to happen in this test because the variable carries meaning. If I'm comparing against a literal value, then I need to read the entire test (or rely on a comment) to extract that meaning. Whenever I read a test and find it confusing, it's usually because I don't understand the intention behind the test.

I don't think I can objectively say this way is better than your suggestion, though, since there are pros and cons to both.

kimjoar · 2016-08-01T16:13:53Z

LGTM

Bargs · 2016-08-03T20:43:38Z

@cjcenizal I think calling the new service appSwitcherState is a little confusing since it affects more than just the app-switcher directive. Until I realized this controls the entire sidebar it seemed really odd that kbn_chrome.js had such intimate knowledge of the appSwitcherState. It seems like chromeState or kbnChromeState would be a more apt name, no?

Bargs · 2016-08-03T21:10:30Z

src/ui/public/chrome/chrome.html

+
+      <!-- Open/close sidebar -->
+      <app-switcher-link
+        app-switcher-link-classes="appSwitcherButton.classes"


I don't think it's necessary to add this custom attribute to the app-switcher-link directive. You can change this line to class="{{appSwitcherButton.classes}}"

Haha, old instinct from writing React components. 😄

Bargs · 2016-08-03T21:58:13Z

@cjcenizal I'm all done reviewing for now. Let me know what you think about the comments above. I've run out of time today, so I'll get to the other x-plugins PR tomorrow.

cjcenizal · 2016-08-04T00:02:57Z

@Bargs Thanks for the excellent feedback. I've addressed most of it. In terms of naming, I think all of the names we're using right now are confusing, so maybe we can clean things up a bit in this PR.

Currently, kbn_chrome.js uses chrome.html as a template. Let's rename that to kbn_chrome.html.

The sidebar is defined by <nav class="app-links-wrapper"> -- the idea of "app links" makes very little sense to me. Let's call this component a "Global Nav", which I think reflects its role better and complements the concept of a Local Nav (@alt74 feel free to chime in).

Let's also encapsulate this markup inside its own directive: <global-nav>. We'll need to rename classes and child directives to reflect this new name. appSwitcherState will become globalNavState.

Thoughts?

Bargs · 2016-08-04T14:06:42Z

+:100: to everything you said about naming. The hardest part about reviewing this PR was wrapping my head around the existing naming schemes.

Bargs · 2016-08-10T23:40:16Z

These links added by x-pack at the bottom aren't showing up in this PR branch (screenshot is from master):

Bargs · 2016-08-10T23:42:54Z

Aside from the issue above, everything looked good. I'll look at it one more time after that's fixed, but you might want to get another set of fresh eyes since a lot has changed since the previous reviewer looked at it. I always get a bit paranoid with big renames.

cjcenizal · 2016-08-11T00:25:14Z

@Bargs Thanks! I'll get another pair of eyes. The X-Plugins don't show up because of all the changes in this PR. Once I get a LGTM I will update the X-Plugins PR to be compatible with this one.

Bargs · 2016-08-11T00:30:06Z

Ahhhh, right, I totally forgot about that. My brain is burnt out for today I think

spalger · 2016-08-11T00:34:37Z

src/ui/public/chrome/directives/global_nav/app_switcher/app_switcher.js

@@ -72,6 +72,15 @@ uiModules
      // links don't cause full-navigation events in certain scenarios
      // so we force them when needed
      this.ensureNavigation = appSwitcherEnsureNavigation;
+
+      $scope.getTooltip = link => {


I would prefer that you stuck with the controller concept here and used this.getTooltip() and switcher.getTooltip() in the view.

I'm not opposed but could you explain the rationale for controller over scope methods?

$scope is a perfectly fine way of exposing values to the view, but it has drawbacks when you start involving child scopes (shadowing, unreflected writes, etc.)

Since around angular 1.5 and the new component type, controllers have just been promoted to be the preferred way to expose values to the view as they provide a namespace preventing the inherited values problem, they can be required by child directives/components, and I think they generally describe their purpose better (the controller controls the view, not the $scope).

spalger · 2016-08-11T00:45:57Z

One note, nothing serious, looks great (to me)!

spalger · 2016-08-15T19:10:25Z

LGTM

…rage. - Add "Expand/Collapse" button to sidebar. - Add appSwitcherState service. - Add support for "classes" attribute to app-switcher-link. - Remove animation from sidebar, and no longer expand on hover. - Add default tooltips for sidebar items.

…e and refactor styles for clarity.

… a rootScope event.

…te event. This prevents accidentally triggering handlers that shouldn't be affected by the appSwitcher position.

- Rename app_switcher -> global_nav, and rename associated services and subcomponents accordingly. - Rename chrome.html -> kbn_chrome.html and create kbn_chrome.less.

…ionality [7467] Add "Collapse/Expand" button to sidebar. Former-commit-id: 962f036

panda01 self-assigned this Jul 13, 2016

Bargs mentioned this pull request Jul 19, 2016

Add the option to turn off main menu animation in kibana-5 #7765

Closed

epixa assigned cjcenizal and unassigned panda01 Jul 22, 2016

cjcenizal force-pushed the 7467/pin-sidebar-functionality branch 4 times, most recently from 1fc33d3 to a45bf55 Compare July 27, 2016 18:32

cjcenizal assigned kimjoar and Bargs Aug 1, 2016

kimjoar reviewed Aug 1, 2016
View reviewed changes

kimjoar removed their assignment Aug 1, 2016

epixa added v5.0.0-beta1 review labels Aug 3, 2016

Bargs reviewed Aug 3, 2016
View reviewed changes

Bargs added updates_needed and removed review labels Aug 3, 2016

cjcenizal force-pushed the 7467/pin-sidebar-functionality branch from 8b2c7b5 to 01cbb47 Compare August 7, 2016 20:46

cjcenizal mentioned this pull request Aug 8, 2016

Move logout and play-circle icons into src/ui/public/icons #7957

Closed

Bargs added updates_needed and removed review labels Aug 10, 2016

cjcenizal assigned spalger Aug 11, 2016

spalger reviewed Aug 11, 2016
View reviewed changes

Bargs removed their assignment Aug 11, 2016

cjcenizal added 7 commits August 15, 2016 12:22

Remove appSwitcherLinkClasses attribute from appSwitcherLink directiv…

a8a99cd

…e and refactor styles for clarity.

Fix data flow to be unidirectional using appSwitcherState service and…

fd2d598

… a rootScope event.

Remove ready:vis event from kbn_chrome. Replace with appSwitcher:upda…

9fbf808

…te event. This prevents accidentally triggering handlers that shouldn't be affected by the appSwitcher position.

Style the log-in screen to remain unaffected by an expanded side nav.

07500a0

Refactor kbn_chrome and app_switcher.

585a84c

- Rename app_switcher -> global_nav, and rename associated services and subcomponents accordingly. - Rename chrome.html -> kbn_chrome.html and create kbn_chrome.less.

In app_switcher.js, expose getTooltip on controller instead of scope.

eb72cb6

cjcenizal force-pushed the 7467/pin-sidebar-functionality branch from 05b5e1a to eb72cb6 Compare August 15, 2016 19:22

cjcenizal removed the updates_needed label Aug 15, 2016

cjcenizal unassigned spalger Aug 15, 2016

cjcenizal merged commit 962f036 into elastic:master Aug 18, 2016

cjcenizal deleted the 7467/pin-sidebar-functionality branch August 18, 2016 19:52

cjcenizal mentioned this pull request Sep 12, 2016

Kill sidebar animation and change main container width instead of translating it #7674

Closed

jbudz mentioned this pull request Sep 22, 2016

5.0.0-alpha1 - UX - sliding sidebar causes mis-clicks on tabs #6856

Closed

cjcenizal mentioned this pull request Oct 5, 2016

left nav detached from the rest of the app #7665

Closed

airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017

Merge pull request elastic#7723 from cjcenizal/7467/pin-sidebar-funct…

a4c7ac5

…ionality [7467] Add "Collapse/Expand" button to sidebar. Former-commit-id: 962f036

ycombinator mentioned this pull request Apr 23, 2018

Show/hide date histogram viz in Discover #17065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[7467] Add "Collapse/Expand" button to sidebar. #7723

[7467] Add "Collapse/Expand" button to sidebar. #7723

cjcenizal commented Jul 12, 2016 •

edited

Loading

kimjoar commented Jul 12, 2016

cjcenizal commented Jul 12, 2016

alt74 commented Jul 12, 2016

Bargs commented Jul 14, 2016

kimjoar Aug 1, 2016

cjcenizal Aug 1, 2016

kimjoar commented Aug 1, 2016

Bargs commented Aug 3, 2016 •

edited

Loading

Bargs Aug 3, 2016

cjcenizal Aug 3, 2016

Bargs commented Aug 3, 2016

cjcenizal commented Aug 4, 2016 •

edited

Loading

Bargs commented Aug 4, 2016

Bargs commented Aug 10, 2016

Bargs commented Aug 10, 2016

cjcenizal commented Aug 11, 2016

Bargs commented Aug 11, 2016

spalger Aug 11, 2016

cjcenizal Aug 11, 2016

spalger Aug 11, 2016

spalger commented Aug 11, 2016 •

edited

Loading

spalger commented Aug 15, 2016

[7467] Add "Collapse/Expand" button to sidebar. #7723

[7467] Add "Collapse/Expand" button to sidebar. #7723

Conversation

cjcenizal commented Jul 12, 2016 • edited Loading

Changes

Expanded and collapsed states

Tooltips on disabled links

kimjoar commented Jul 12, 2016

cjcenizal commented Jul 12, 2016

alt74 commented Jul 12, 2016

Bargs commented Jul 14, 2016

kimjoar Aug 1, 2016

Choose a reason for hiding this comment

cjcenizal Aug 1, 2016

Choose a reason for hiding this comment

kimjoar commented Aug 1, 2016

Bargs commented Aug 3, 2016 • edited Loading

Bargs Aug 3, 2016

Choose a reason for hiding this comment

cjcenizal Aug 3, 2016

Choose a reason for hiding this comment

Bargs commented Aug 3, 2016

cjcenizal commented Aug 4, 2016 • edited Loading

Bargs commented Aug 4, 2016

Bargs commented Aug 10, 2016

Bargs commented Aug 10, 2016

cjcenizal commented Aug 11, 2016

Bargs commented Aug 11, 2016

spalger Aug 11, 2016

Choose a reason for hiding this comment

cjcenizal Aug 11, 2016

Choose a reason for hiding this comment

spalger Aug 11, 2016

Choose a reason for hiding this comment

spalger commented Aug 11, 2016 • edited Loading

spalger commented Aug 15, 2016

cjcenizal commented Jul 12, 2016 •

edited

Loading

Bargs commented Aug 3, 2016 •

edited

Loading

cjcenizal commented Aug 4, 2016 •

edited

Loading

spalger commented Aug 11, 2016 •

edited

Loading