[Add panel flyout] Moving create new to the top of SavedObjectFinder #56428

majagrubic · 2020-01-30T19:29:59Z

Summary

To draw more attention to the "Create New" button in add panel flyout on the dashboard, we want to move this button from the bottom to the top, as explored in this prototype:
https://www.figma.com/file/EwodWYRUIjbNvYoi9G4Hnh/Lensboard?node-id=611%3A0

This PR removes the footer in the AddPanelFlyout and adds Create New button as a child of SavedObjectFinder component.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

majagrubic · 2020-01-31T18:44:08Z

@elasticmachine merge upstream

elasticmachine · 2020-01-31T20:03:32Z

Pinging @elastic/kibana-app (Team:KibanaApp)

majagrubic · 2020-01-31T21:58:29Z

...s/embeddable/public/lib/panel/panel_header/panel_actions/add_panel/add_panel_flyout.test.tsx

@@ -101,14 +109,14 @@ test('selecting embeddable in "Create new ..." list calls createNewEmbeddable()'
  };
  const container = new HelloWorldContainer(input, { getEmbeddableFactory } as any);
  const onClose = jest.fn();
-  const component = mount<AddPanelFlyout>(
+  const component = mount(


I'm not super convinced this test is useful anymore. It tests interaction between components, which is what functional tests are better suited for. But leaving it here for now.

lizozom · 2020-02-02T11:58:55Z

@elasticmachine merge upstream

lizozom

After merging upstream, and pulling down the changes, the button doesn't do anything.
I'm also really surprised the tests are passing. 🤔

streamich · 2020-02-03T10:34:11Z

...s/embeddable/public/lib/panel/panel_header/panel_actions/add_panel/add_panel_flyout.test.tsx

 // @ts-ignore
 import { findTestSubject } from '@elastic/eui/lib/test';

-// eslint-disable-next-line
-import { coreMock } from '../../../../../../../../core/public/mocks';
+// @ts-ignore


Can we please remove this // @ts-ignore and use TypeScript casting instead, if needed.

streamich · 2020-02-03T10:34:27Z

...lib/panel/panel_header/panel_actions/add_panel/tests/saved_object_finder_create_new.test.tsx

+    expect(wrapper.find(EuiPopover).length).toEqual(1);
+    const menuPanel = wrapper.find(EuiContextMenuPanel);
+    expect(menuPanel.length).toEqual(1);
+    // @ts-ignore


flash1293

LGTM, it seems to work fine for me.

@lizozom The button should open a popover with a single entry "Visualization" - works on my machine (TM)

@majagrubic I'm not sure whether I like the fact that it opens the popover to the top, as a user I would have expected to open to the bottom. Like this it kind of looks like a tooltip, not like context menu. Just my 2 cents, cc @cchaos

flash1293 · 2020-02-03T10:57:46Z

...lib/panel/panel_header/panel_actions/add_panel/tests/saved_object_finder_create_new.test.tsx

+    }
+
+    const wrapper = shallow(<SavedObjectFinderCreateNew menuItems={items} />);
+    expect(wrapper).toMatchSnapshot();


Nit: Do we really need the snapshot test here? Seems like the code below is already validating the output quite well.

lizozom · 2020-02-03T11:22:53Z

@flash1293 @majagrubic sorry, it does work.
Testing this, I actually didn't realize this was the menu!
I thought it was just a tooltip, especially with the arrow pointing down on the button.

IMO such a menu has to open down, and not up. 👍 on @flash1293 comment

@ryankeairns

lizozom

Overall LGTM
Assuming UX + ts issues are addressed

ryankeairns · 2020-02-03T14:31:46Z

Thanks for making this change, Maja!

+1 to opening the popover down. Specifically, anchorPosition="downRight" since this is along the right edge of the screen.

Related, can we now add Lens to this menu?

flash1293 · 2020-02-03T15:59:03Z

@ryankeairns Lens is part of the "Visualization" option - if you click it you will get the grid with all visualization types including Lens:

I realize this isn't ideal because it adds another click till the user can work on their visualization, +1 for showing Lens in this menu directly. I think that's worth a second PR though.

ryankeairns · 2020-02-03T17:08:36Z

@flash1293 of course, my mistake! I agree we should both consider adding it and doing so as a follow up PR. I'll create an issue.

* Move saved object modal into new platform * Fix TS * Revert "Fix TS" This reverts commit f2f9f5e. * Revert "Move saved object modal into new platform" This reverts commit d0f0ea6. # Conflicts: # src/legacy/core_plugins/kibana/public/discover/np_ready/angular/discover.js * Move save_object_save_modal * Move show_saved_object_save_modal.tsx * Move save_object_finder.tsx * Remove unused export * Pass I18nContext to showSaveModal * Update i18n ids * Fix map save * Refactoring * Load styles * Revert importing styles * Update snapshot * Update snapshot * Structural refactoring * Fix path Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

majagrubic · 2020-02-10T10:55:07Z

@elasticmachine merge upstream

Removing faulty rebase imports Fixing unresolved conflict Removing faulty merge files Removing faulty import Readd accidentally added file

majagrubic · 2020-02-10T12:53:06Z

Thanks everyone. I've addressed your comments and fixed the popup.

kibanamachine · 2020-02-10T12:58:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9005424

History

💛 Build #25521 was flaky 4e83f58
💚 Build #23910 succeeded 68e442a
💚 Build #23845 succeeded 59d36b6
💚 Build #23795 succeeded 7246f15
💔 Build #23692 failed 45ac37c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

lizozom

Thanks for fixing that UI!
Tested and LGTM

* master: (27 commits) Include actions new platform plugin for codeowners (elastic#57252) [APM][docs] 7.6 documentation updates (elastic#57124) Expressions refactor (elastic#54342) [ML] New Platform server shim: update annotation routes to use new platform router (elastic#57067) Remove injected ui app vars from Canvas (elastic#56190) update max_anomaly_score route schema to handle possible undefined values (elastic#57339) [Add panel flyout] Moving create new to the top of SavedObjectFinder (elastic#56428) Add mock of a legacy ui api to re-enable Canvas storybook (elastic#56673) [monitoring] Removes noisy event received log (elastic#57275) Remove use of copied MANAGEMENT_BREADCRUMBS and use `setBreadcrumbs` from management section's mount (elastic#57324) Advanced Settings management app to kibana platform plugin (elastic#56931) [ML] New Platform server shim: update recognize modules routes to use new platform router (elastic#57206) [ML] Fix overall stats for saved search on the Data Visualizer page (elastic#57312) [ML] [NP] Removing ui imports (elastic#56358) [SIEM] Fixes failing Cypress tests (elastic#57202) Create observability CODEOWNERS reference (elastic#57109) fix results service schema (elastic#57217) don't register a wrapper if browser side function exists. (elastic#57196) Ui Actions explorer example (elastic#57006) Fix update alert API to still work when AAD is out of sync (elastic#57039) ...

…56428) (#57360) * [Add panel flyout] Moving create new to the top of SavedObjectFinder * [Add panel flyout] Moving create new to the top of SavedObjectFinder * Fixing failing unit test * Readd missing test * [NP] Move saved object modal into new platform (#56383) * Move saved object modal into new platform * Fix TS * Revert "Fix TS" This reverts commit f2f9f5e. * Revert "Move saved object modal into new platform" This reverts commit d0f0ea6. # Conflicts: # src/legacy/core_plugins/kibana/public/discover/np_ready/angular/discover.js * Move save_object_save_modal * Move show_saved_object_save_modal.tsx * Move save_object_finder.tsx * Remove unused export * Pass I18nContext to showSaveModal * Update i18n ids * Fix map save * Refactoring * Load styles * Revert importing styles * Update snapshot * Update snapshot * Structural refactoring * Fix path Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * Applying PR comments Removing faulty rebase imports Fixing unresolved conflict Removing faulty merge files Removing faulty import Readd accidentally added file * Removing unnecessary eslint-ignore Co-authored-by: Maryia Lapata <mary.lopato@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Maryia Lapata <mary.lopato@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

majagrubic marked this pull request as ready for review January 31, 2020 20:02

majagrubic requested a review from a team as a code owner January 31, 2020 20:02

majagrubic added v8.0.0 v7.6.0 v7.7.0 release_note:roadmap Team:Visualizations Visualization editors, elastic-charts and infrastructure and removed v7.6.0 labels Jan 31, 2020

majagrubic requested a review from a team January 31, 2020 20:03

majagrubic force-pushed the add-flyout branch 2 times, most recently from 498c8ff to 59d36b6 Compare January 31, 2020 21:54

majagrubic commented Jan 31, 2020

View reviewed changes

majagrubic requested a review from flash1293 January 31, 2020 22:31

lizozom reviewed Feb 3, 2020

View reviewed changes

streamich reviewed Feb 3, 2020

View reviewed changes

flash1293 approved these changes Feb 3, 2020

View reviewed changes

lizozom approved these changes Feb 3, 2020

View reviewed changes

majagrubic requested review from a team as code owners February 10, 2020 10:36

majagrubic removed request for a team February 10, 2020 10:40

majagrubic force-pushed the add-flyout branch from 2e656bb to 64b36aa Compare February 10, 2020 10:52

Maja Grubic and others added 5 commits February 10, 2020 10:52

[Add panel flyout] Moving create new to the top of SavedObjectFinder

03e3487

Fixing failing unit test

c2c0c42

Readd missing test

6a8ba78

[Add panel flyout] Moving create new to the top of SavedObjectFinder

b98cd2c

majagrubic force-pushed the add-flyout branch 2 times, most recently from bf2a763 to 4e83f58 Compare February 10, 2020 11:23

Maja Grubic added 2 commits February 10, 2020 11:24

Applying PR comments

4e83f58

Removing faulty rebase imports Fixing unresolved conflict Removing faulty merge files Removing faulty import Readd accidentally added file

Removing unnecessary eslint-ignore

9005424

majagrubic requested review from streamich and flash1293 February 10, 2020 12:52

majagrubic requested a review from lizozom February 10, 2020 13:01

lizozom approved these changes Feb 11, 2020

View reviewed changes

streamich approved these changes Feb 11, 2020

View reviewed changes

majagrubic merged commit 7b6db2a into elastic:master Feb 11, 2020

majagrubic deleted the add-flyout branch February 11, 2020 18:10

majagrubic mentioned this pull request Feb 11, 2020

[7.x] [Add panel flyout] Moving create new to the top of SavedObjectFinder (#56428) #57360

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Add panel flyout] Moving create new to the top of SavedObjectFinder #56428

[Add panel flyout] Moving create new to the top of SavedObjectFinder #56428

majagrubic commented Jan 30, 2020 •

edited

Loading

majagrubic commented Jan 31, 2020

elasticmachine commented Jan 31, 2020

majagrubic Jan 31, 2020 •

edited

Loading

lizozom commented Feb 2, 2020

lizozom left a comment •

edited

Loading

streamich Feb 3, 2020

streamich Feb 3, 2020

flash1293 left a comment

flash1293 Feb 3, 2020

lizozom commented Feb 3, 2020 •

edited

Loading

lizozom left a comment

ryankeairns commented Feb 3, 2020

flash1293 commented Feb 3, 2020

ryankeairns commented Feb 3, 2020 •

edited

Loading

majagrubic commented Feb 10, 2020

majagrubic commented Feb 10, 2020 •

edited

Loading

kibanamachine commented Feb 10, 2020

lizozom left a comment

[Add panel flyout] Moving create new to the top of SavedObjectFinder #56428

[Add panel flyout] Moving create new to the top of SavedObjectFinder #56428

Conversation

majagrubic commented Jan 30, 2020 • edited Loading

Summary

Checklist

For maintainers

majagrubic commented Jan 31, 2020

elasticmachine commented Jan 31, 2020

majagrubic Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

lizozom commented Feb 2, 2020

lizozom left a comment • edited Loading

Choose a reason for hiding this comment

streamich Feb 3, 2020

Choose a reason for hiding this comment

streamich Feb 3, 2020

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Feb 3, 2020

Choose a reason for hiding this comment

lizozom commented Feb 3, 2020 • edited Loading

lizozom left a comment

Choose a reason for hiding this comment

ryankeairns commented Feb 3, 2020

flash1293 commented Feb 3, 2020

ryankeairns commented Feb 3, 2020 • edited Loading

majagrubic commented Feb 10, 2020

majagrubic commented Feb 10, 2020 • edited Loading

kibanamachine commented Feb 10, 2020

💚 Build Succeeded

History

lizozom left a comment

Choose a reason for hiding this comment

majagrubic commented Jan 30, 2020 •

edited

Loading

majagrubic Jan 31, 2020 •

edited

Loading

lizozom left a comment •

edited

Loading

lizozom commented Feb 3, 2020 •

edited

Loading

ryankeairns commented Feb 3, 2020 •

edited

Loading

majagrubic commented Feb 10, 2020 •

edited

Loading