[UI Framework] Add KuiContextMenu. #14183

cjcenizal · 2017-09-26T20:11:11Z

Ports over many improvements from K7 (#14108).

Update KuiPopover to use K7 code.
Add KuiPanelSimple for use within KuiPopover; it's just the K7 KuiPanel renamed.
Refactor/rewrite KuiExpression and KuiExpressionButton to depend upon KuiPopover.

To review:

Take a look at the KuiPanelSimple, KuiPopover, KuiContextMenu, and KuiExpression examples and source code.
Ignore most changes to files in the ui_framework/src/global_styling directory and changes to the files that are not related to the above components.
Check out Dashboard panels to make sure the popover still works correctly.

cjcenizal · 2017-09-27T00:18:38Z

@stacey-gammon Could you take a look at this and advise me on how this affects #13853? There was a merge conflict with the changes you made to popover.scss, but I just overwrote the changes you made. Maybe we can pair up and work through the changes to make sure I haven't broken anything for you.

@timroes Could you take a look and let me know what you think of these changes in terms of accessibility?

A more general code review would also be welcome from each of you if you have time.

kimjoar · 2017-09-27T10:42:06Z

I only skimmed through some of the changes, but in general it looks like we need more component tests here. Some of these components are fairly complex.

cjcenizal · 2017-09-28T03:36:50Z

@kjbekkelund You got me! Thanks for keeping me honest. I just pushed a bunch of tests. Would you mind taking a look?

@timroes I also made some tweaks to the KuiCodeEditor tests. Could you look them over please?

@stacey-gammon I also updated the snapshot for the dashboard_panel test. Could you take a look and let me know what you think?

Thanks, everyone!

kimjoar

This is touching a ton of files and has lots of little changes, so it's not the easiest to review. But the tests are definitely looking better now. I haven't gone through every component as I just didn't have time right now.

kimjoar · 2017-09-29T18:39:42Z

ui_framework/src/components/popover/popover_title.js

+
+  const classes = classNames(
+    {
+      'kuiPopoverTitle': children,


children isn't clear here. It needs a test, plus we should maybe change this to children != null or something like that? It's difficult to understand the intention right now

Great catch... I don't think this is necessary at all.

cjcenizal · 2017-09-29T19:13:17Z

@kjbekkelund Thanks man. I'm going to try to break this PR up a bit to make it easier to review. I'll let you know when it's been deflated.

timroes · 2017-10-02T09:26:27Z

This really is a rather large PR and it looks fine. I skipped through the code and as @kjbekkelund already mentioned, it's touching quite some files :-) So I also feel hard to just say, everything is fine (though it looks like everything is working).

cjcenizal · 2017-10-02T15:58:45Z

@kjbekkelund @timroes I've extracted out as much as I possibly can from this PR. Could you take another look? It's still a daunting PR based solely on size, but I think it becomes easier if you review one component at a time:

KuiPanelSimple
KuiPopover
3 KuiContextMenu
KuiExpression

@stacey-gammon Could you take a look at the Dashboard panel and verify that it still functions correctly?

kimjoar · 2017-10-02T17:52:47Z

The last commit (Remove unnecessary logic from KuiPopoverTitle.) added tons of generated code +7,102 −8, which doesn't seem relevant to this PR(?)

cjcenizal · 2017-10-02T17:55:24Z

Good catch @kjbekkelund, fixed. Sorry about that.

stacey-gammon · 2017-10-02T19:51:52Z

I think there is a bug when you click on Edit visualization. I get a quick error that disappears on the screen - something about can't read style of null, then redirects to edit the visualization. Doesn't seem to happen in master.

Otherwise, it works great!

cjcenizal · 2017-10-03T20:18:22Z

@stacey-gammon great catch, fixed!

weltenwort

Wow, the UX of that is awesome!

I have a few suggestions on how to improve the React components' apis, mainly of KuiContextMenu itself:

Split out the transition part into a HorizontalSwipeTransition component or similar.
Replace the confusing idToPanelMap and idToPreviousPanelMap props with a flattened panels map, where each panel references its children via id
Use an array of panel ids as the currentPanelPath state so we can derive the "previous" panel from the state.

Let's maybe talk about this synchronously some time?

cjcenizal · 2017-10-06T22:29:04Z

@weltenwort I just pushed a commit which uses a flat panels prop as you suggest. Great idea!

cjcenizal · 2017-10-09T18:01:08Z

@weltenwort Thanks for all of your help! I like your suggestion but I think I'll need some help implementing it. The problem we need to overcome is that the application state doesn't map cleanly to the visual state due to the fade-out transition when the the ContextMenu closes. In the owner, state.isPopoverOpen determines when the the ContextMenu is visible. If we simply don't render the ContextMenu when this is false then the ContextMenu will abruptly disappear. But we don't want that; we want the CSS transition to take effect and gently fade it out.

We could solve this by adding a onCloseComplete prop or something, so the owner can re-render when the transition is done, but I'm not sure it's worth it. What do you think?

weltenwort · 2017-10-09T18:15:12Z

@cjcenizal, I think what you are looking for is https://github.com/reactjs/react-transition-group with its <TransitionGroup> and <CSSTransition> components. The <TransitionGroup> keeps the "exiting" components around until the transition is done.

cjcenizal · 2017-10-09T18:16:44Z

Ah ha! Thanks man!

- Update KuiPopover to use K7 code. - Add KuiPanelSimple for use within KuiPopover; it's just the K7 KuiPanel renamed. - Refactor/rewrite KuiExpression and KuiExpressionButton to depend upon KuiPopover. - Add K7 shadow mixins and size and z-index vars to global_styling.

- Refactor tests to use a test subject selector to locate the hint element. - Refine tests to leverage snapshots instead of DOM assertions.

- Update snapshot. - Update Jest config to make snapshotComponent available to Kibana src.

- Move keyboard navigation logic into KuiContextPanel. - Add example to doc site. - Update tests.

…ontextMenu.

…ard.

…a panels prop.

…tMenuPanel. - Rename 'current panel' to 'incoming panel' for cohesion with 'outgoing panel.'

… and update tests.

cjcenizal · 2017-10-11T16:09:37Z

@weltenwort I looked into CSSTransition and the related components but I wasn't sure how we could implement them in a way that made isVisible unnecessary. Maybe we can work on this together later?

I'm going to merge this because complementary parts of it have been looked over by different engineers, so I feel comfortable with the code's correctness and quality at this point.

* Add KuiContextMenu. - Update KuiPopover to use K7 code. - Add KuiPanelSimple for use within KuiPopover; it's just the K7 KuiPanel renamed. - Refactor/rewrite KuiExpression and KuiExpressionButton to depend upon KuiPopover. - Add K7 shadow mixins and size and z-index vars to global_styling. * Update Dashboard panel to use KuiContextMenu. - Fix reloading issue when editing a visualization from within a dashboard. * Completely refactor KuiContextMenu to enable a single panel. - Move keyboard navigation logic into KuiContextPanel. - Set focus on the item which shows the panel we're leaving within KuiContextMenu. - Remove unnecessary logic from KuiPopoverTitle. - Replace confusing idToPanelMap and idToPreviousPanelIdMap props with a panels prop. - Replace panelRef prop with onHeightChange prop. - Migrate transition state and logic from KuiContextMenu into KuiContextMenuPanel. - Rename 'current panel' to 'incoming panel' for cohesion with 'outgoing panel.' - Map panel items to panels up-front. - Convert maps from state variables into instance variables.

cjcenizal added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. v6.1.0 v7.0.0 labels Sep 26, 2017

cjcenizal force-pushed the improvement/accessible-context-menu branch 2 times, most recently from cd962b9 to a35df3a Compare September 27, 2017 00:15

cjcenizal requested review from stacey-gammon and timroes September 27, 2017 00:15

cjcenizal force-pushed the improvement/accessible-context-menu branch 2 times, most recently from c58e181 to 0716794 Compare September 28, 2017 03:43

cjcenizal requested a review from kimjoar September 29, 2017 01:32

kimjoar reviewed Sep 29, 2017

View reviewed changes

cjcenizal mentioned this pull request Sep 29, 2017

[UI Framework] Rename all $global-prefixed SCSS vars to use $kui-prefix for compatibility with K7. #14222

Merged

cjcenizal force-pushed the improvement/accessible-context-menu branch from c8957b4 to 0c367d5 Compare September 29, 2017 23:00

cjcenizal mentioned this pull request Sep 29, 2017

Add snapshotComponent test service for Jest. #14226

Merged

cjcenizal force-pushed the improvement/accessible-context-menu branch from 7c511ab to 4702788 Compare October 2, 2017 15:55

cjcenizal force-pushed the improvement/accessible-context-menu branch from 4702788 to 98a91bc Compare October 2, 2017 17:54

cjcenizal requested review from kimjoar and weltenwort October 5, 2017 15:54

weltenwort reviewed Oct 6, 2017

View reviewed changes

cjcenizal added 17 commits October 10, 2017 17:32

Add dark theme support to KuiContextMenu.

9f29d3d

Add snapshotComponent test service. Update KuiCodeEditor test to use it.

cb47f75

- Refactor tests to use a test subject selector to locate the hint element. - Refine tests to leverage snapshots instead of DOM assertions.

Add tests for KuiContextMenu.

77c3808

Update dashboard_panel test to use snapshotComponent.

8abd991

- Update snapshot. - Update Jest config to make snapshotComponent available to Kibana src.

Completely refactor KuiContextMenu to enable a single panel.

c1bc5f3

- Move keyboard navigation logic into KuiContextPanel. - Add example to doc site. - Update tests.

Update Dashboard panel to use KuiContextMenu.

f56a5b4

Set focus on the item which shows the panel we're leaving within KuiC…

171ddf8

…ontextMenu.

Remove unnecessary logic from KuiPopoverTitle.

2489e17

Fix reloading issue when editing a visualization from within a dashbo…

31d8039

…ard.

Replace confusing idToPanelMap and idToPreviousPanelIdMap props with …

a230475

…a panels prop.

Replace panelRef prop with onHeightChange prop.

22930ad

Update tests.

53acd49

Migrate transition state and logic from KuiContextMenu into KuiContex…

8833eca

…tMenuPanel. - Rename 'current panel' to 'incoming panel' for cohesion with 'outgoing panel.'

Map panel items to panels up-front.

4f8a09e

Convert maps from state variables into instance variables.

f7ac90f

Fix bug with determining isTransitioning state in KuiContextMenuPanel…

9ab0a13

… and update tests.

cjcenizal force-pushed the improvement/accessible-context-menu branch from 5796c71 to 9ab0a13 Compare October 11, 2017 04:08

cjcenizal merged commit abcccd0 into elastic:master Oct 11, 2017

cjcenizal deleted the improvement/accessible-context-menu branch October 11, 2017 16:11

cjcenizal mentioned this pull request Oct 11, 2017

[6.x] [UI Framework] Add KuiContextMenu. (#14183) #14427

Merged

cjcenizal mentioned this pull request Oct 12, 2017

[UI Framework] Improve KuiContextMenu keyboard navigation UX #14434

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UI Framework] Add KuiContextMenu. #14183

[UI Framework] Add KuiContextMenu. #14183

cjcenizal commented Sep 26, 2017 •

edited

Loading

cjcenizal commented Sep 27, 2017

kimjoar commented Sep 27, 2017

cjcenizal commented Sep 28, 2017

kimjoar left a comment

kimjoar Sep 29, 2017

cjcenizal Sep 30, 2017

cjcenizal commented Sep 29, 2017

timroes commented Oct 2, 2017

cjcenizal commented Oct 2, 2017

kimjoar commented Oct 2, 2017

cjcenizal commented Oct 2, 2017

stacey-gammon commented Oct 2, 2017

cjcenizal commented Oct 3, 2017

weltenwort left a comment

cjcenizal commented Oct 6, 2017

cjcenizal commented Oct 9, 2017

weltenwort commented Oct 9, 2017

cjcenizal commented Oct 9, 2017

cjcenizal commented Oct 11, 2017 •

edited

Loading

[UI Framework] Add KuiContextMenu. #14183

[UI Framework] Add KuiContextMenu. #14183

Conversation

cjcenizal commented Sep 26, 2017 • edited Loading

cjcenizal commented Sep 27, 2017

kimjoar commented Sep 27, 2017

cjcenizal commented Sep 28, 2017

kimjoar left a comment

Choose a reason for hiding this comment

kimjoar Sep 29, 2017

Choose a reason for hiding this comment

cjcenizal Sep 30, 2017

Choose a reason for hiding this comment

cjcenizal commented Sep 29, 2017

timroes commented Oct 2, 2017

cjcenizal commented Oct 2, 2017

kimjoar commented Oct 2, 2017

cjcenizal commented Oct 2, 2017

stacey-gammon commented Oct 2, 2017

cjcenizal commented Oct 3, 2017

weltenwort left a comment

Choose a reason for hiding this comment

cjcenizal commented Oct 6, 2017

cjcenizal commented Oct 9, 2017

weltenwort commented Oct 9, 2017

cjcenizal commented Oct 9, 2017

cjcenizal commented Oct 11, 2017 • edited Loading

cjcenizal commented Sep 26, 2017 •

edited

Loading

cjcenizal commented Oct 11, 2017 •

edited

Loading