[Dashboard] Deangularize Nav Bar #55443

majagrubic · 2020-01-21T18:09:37Z

Summary

Removing Angular wrapper around Angular NavBar in Dashboard app. Some refactoring of existing logic to support the new DOM structure.
I've tested this in the following scenarios:

Updating/deleting/changing saved queries
Searching
Adding/removing/enabling/disabling filters
Updating timerange

Some additional comments below to help review this

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

src/plugins/data/public/ui/filter_bar/filter_bar.tsx

majagrubic · 2020-01-24T14:56:55Z

src/plugins/data/public/ui/search_bar/search_bar.tsx

  };

  private services = this.props.kibana.services;
  private savedQueryService = this.services.data.query.savedQueries;
  public filterBarRef: Element | null = null;
  public filterBarWrapperRef: Element | null = null;

-  public static getDerivedStateFromProps(nextProps: SearchBarProps, prevState: State) {


This was some very convoluted for updating state from props. I think a simpler version can be done in componentDidUpdate.

majagrubic · 2020-01-24T15:00:51Z

src/plugins/navigation/public/top_nav_menu/top_nav_menu.tsx

@@ -63,13 +64,14 @@ export function TopNavMenu(props: TopNavMenuProps) {
  }

  function renderLayout() {
+    const className = props.noPadding ? undefined : 'kbnTopNavMenu';


kbnTopNavMenu has some top/bottom padding:

kibana/src/plugins/navigation/public/top_nav_menu/_index.scss

Line 5 in daac96c

padding: $euiSizeS 0px $euiSizeXS;

which, with the new DOM structure, was causing Dashboard in the fullscreen mode to have some empty space between the top of the screen and the first row of visualizations. We need to disable paddings in fullscreen mode.

I think the same problem should exist for visualize?
Can we implement it in the SCSS file in such a way that solves the problem for both?

src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx

elasticmachine · 2020-01-28T10:51:10Z

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

majagrubic · 2020-01-28T20:54:00Z

Jenkins, test this

…#55443

lizozom

Overall looks good.
There's a small regression with saved queries - on master, loading a saved query immediatelly applies it, while on your PR - you have to click "Update" to apply it.

It's not a blocker for this PR of course, but I would recommend using the default behaviors available in this PR, to delete saved query handling (as well as other default behaviors) from dashboard code.

lizozom · 2020-01-29T16:33:54Z

src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx

@@ -490,11 +544,10 @@ export class DashboardAppController {
        },
        queryFilter.getGlobalFilters()
      );
-      // Making this method sync broke the updates.
-      // Temporary fix, until we fix the complex state in this file.
      setTimeout(() => {
        queryFilter.setFilters(queryFilter.getGlobalFilters());


Can you do a replace all of queryFilter to filterManager?

lizozom · 2020-01-29T16:42:08Z

src/plugins/navigation/public/top_nav_menu/top_nav_menu.tsx

@@ -63,13 +64,14 @@ export function TopNavMenu(props: TopNavMenuProps) {
  }

  function renderLayout() {
+    const className = props.noPadding ? undefined : 'kbnTopNavMenu';


I think the same problem should exist for visualize?
Can we implement it in the SCSS file in such a way that solves the problem for both?

test/functional/apps/management/_kibana_settings.js

majagrubic · 2020-01-29T17:37:36Z

Not sure why I can't reply below the comment, but re:
#55443 (comment)

Visualize doesn't have fullscreen mode (where we're are hiding the top nav), so I don't think it should be an issue.

majagrubic · 2020-01-29T21:22:54Z

Overall looks good.
There's a small regression with saved queries - on master, loading a saved query immediatelly applies it, while on your PR - you have to click "Update" to apply it.

Great catch. Looks like the watcher receives undefined as a new query value and does nothing.

flash1293

Left two small nits, but they are not blockers.

I noticed that in some cases the filter state update doesn't seem to get propagated correctly.

Create dashboard
Add visualization
Save current query (include time range)
Add a filter pill
The filter pill appears and disappears

flash1293 · 2020-01-30T09:26:56Z

src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx

@@ -254,6 +258,41 @@ export class DashboardAppController {
      };
    };

+    $scope.showFilterBar = () =>


Do we need to put this on the scope? Seems like it not used anymore from within angular

flash1293 · 2020-01-30T09:28:11Z

src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx

@@ -441,22 +488,25 @@ export class DashboardAppController {
      }
    };

-    $scope.updateQueryAndFetch = function({ query, dateRange }) {


Same here - seems like this is only used from within react

Fixing type for notifications service Fixing notifications service types Resolving all type errors Removing unnecessary watch Refactoring Do not show menu bar in fullscreen mode Fix flickering on FilterBar popover opening Removing directive from application Update navbar after saving new query Adding missing navbar update Clear query when clearing saved query Cleaning up convoluted search bar logic Fixing typecheck Removing dirty state tampering Renaming history > hashHistory Clear query bar when saved query cleared Fix filterStateManager Update navbar after updating filters Fixing issues after rebase Update dateRange from props Update props from query Replace hashHistory with history Replacing stateless TopNav component with the stateful one Fix type error

kibanamachine · 2020-01-30T13:22:55Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: 09a463f
Flaky suites:
- xpack-firefoxSmoke

History

💚 Build #23322 succeeded 103a2f645b6b3be2bb72f4a614648ef4a710fa51
💚 Build #23308 succeeded a4b0a6a240181aa474b4a7bfad7d04f9c5bc3772
💔 Build #23150 failed 28017a0160b001f4067b697e4d8b9e8d5f121a47
💔 Build #23138 failed 28017a0160b001f4067b697e4d8b9e8d5f121a47
💚 Build #23025 succeeded c85b0e32b22fa790d5115372a3bf33f4048bd824

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

majagrubic · 2020-01-30T13:59:46Z

Hey @lizozom and @flash1293 thanks for reviews. I am going to pause this for now until
#56160 is merged and hopefully I can offload more state to the search bar. There are some race conditions here when state is updated in two different places which make this hard to debug, and it's not high priority change.

lizozom · 2020-01-30T13:59:53Z

@majagrubic, it might be only my laptop, but it seems to me the rendering is less performant.
Could it be due to changing from getDerivedStateFromProps to componentDidUpdate?

@kertal

* Clean up discover * Clean up visualize * Clean up dashboard * use-default-behaviors * ts * Discover interval changing * timerange for interval definition in editor * ts * Revert most of changes to dashboard app because of changes in #55443 * Fix spaces * Revert editor to scope PR! * typo * keep savedQuery state in create search bar * update saved query to save it with the state * revert all dashboard changes * saved queries * @kertal code review * fix applying filters from histogram * @Dosant code review * Merge changes from #56643 to handle saved query errors Fix bug where saved query clean does not reset query * change string path * if * Extract useFilterManager and useTimefilter * Split useSavedQuery and restore capability of changing saved query in URL * Added some tests * context view * Remove useMemo * spaces * Support filter intial values Improve useSavedQuery hook in terface Co-authored-by: Elastic Machine <[email protected]>

@kertal

* Clean up discover * Clean up visualize * Clean up dashboard * use-default-behaviors * ts * Discover interval changing * timerange for interval definition in editor * ts * Revert most of changes to dashboard app because of changes in elastic#55443 * Fix spaces * Revert editor to scope PR! * typo * keep savedQuery state in create search bar * update saved query to save it with the state * revert all dashboard changes * saved queries * @kertal code review * fix applying filters from histogram * @Dosant code review * Merge changes from elastic#56643 to handle saved query errors Fix bug where saved query clean does not reset query * change string path * if * Extract useFilterManager and useTimefilter * Split useSavedQuery and restore capability of changing saved query in URL * Added some tests * context view * Remove useMemo * spaces * Support filter intial values Improve useSavedQuery hook in terface Co-authored-by: Elastic Machine <[email protected]>

@kertal

* Clean up discover * Clean up visualize * Clean up dashboard * use-default-behaviors * ts * Discover interval changing * timerange for interval definition in editor * ts * Revert most of changes to dashboard app because of changes in #55443 * Fix spaces * Revert editor to scope PR! * typo * keep savedQuery state in create search bar * update saved query to save it with the state * revert all dashboard changes * saved queries * @kertal code review * fix applying filters from histogram * @Dosant code review * Merge changes from #56643 to handle saved query errors Fix bug where saved query clean does not reset query * change string path * if * Extract useFilterManager and useTimefilter * Split useSavedQuery and restore capability of changing saved query in URL * Added some tests * context view * Remove useMemo * spaces * Support filter intial values Improve useSavedQuery hook in terface Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

majagrubic changed the title ~~Deangularize filter bar~~ [Dashboard] Deangularize Nav Bar Jan 21, 2020

majagrubic force-pushed the deangularize-filter-bar branch from 232eb7f to 3fbeb0f Compare January 22, 2020 10:17

majagrubic commented Jan 24, 2020

View reviewed changes

src/plugins/data/public/ui/filter_bar/filter_bar.tsx Outdated Show resolved Hide resolved

majagrubic commented Jan 24, 2020

View reviewed changes

majagrubic force-pushed the deangularize-filter-bar branch 2 times, most recently from 8380ff8 to 5563f41 Compare January 27, 2020 11:14

lizozom reviewed Jan 27, 2020

View reviewed changes

src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx Outdated Show resolved Hide resolved

majagrubic force-pushed the deangularize-filter-bar branch from f58095d to cefc85c Compare January 28, 2020 09:32

majagrubic added v8.0.0 v7.7.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jan 28, 2020

majagrubic added release_note:skip Skip the PR/issue when compiling release notes Feature:Dashboard Dashboard related features labels Jan 28, 2020

majagrubic marked this pull request as ready for review January 28, 2020 11:07

majagrubic requested a review from a team January 28, 2020 11:07

majagrubic requested a review from a team as a code owner January 28, 2020 11:07

majagrubic requested review from flash1293 and lizozom January 28, 2020 11:07

lizozom pushed a commit to lizozom/kibana that referenced this pull request Jan 29, 2020

Revert most of changes to dashboard app because of changes in elastic…

469f41c

…#55443

Dosant mentioned this pull request Jan 29, 2020

[Bugfix][State Management] 'state:storeInSessionStorage' doesn’t take effect for dashboard without full page reload #56284

Merged

7 tasks

majagrubic force-pushed the deangularize-filter-bar branch 2 times, most recently from a4b0a6a to 103a2f6 Compare January 29, 2020 16:07

lizozom reviewed Jan 29, 2020

View reviewed changes

flash1293 requested changes Jan 30, 2020

View reviewed changes

Maja Grubic added 4 commits January 30, 2020 10:28

Refactor search bar logic a bit

c57762b

Remove setting of popover own focus

34e32b2

Removing setting of absolute time range

7b6787f

Execute query as soon as saved query is updated

09a463f

majagrubic force-pushed the deangularize-filter-bar branch from 103a2f6 to 09a463f Compare January 30, 2020 12:01

majagrubic closed this Mar 11, 2020

majagrubic mentioned this pull request Apr 1, 2020

[Dashboard] Deangularize navbar, attempt nr. 2 #61611

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] Deangularize Nav Bar #55443

[Dashboard] Deangularize Nav Bar #55443

majagrubic commented Jan 21, 2020 •

edited

Loading

majagrubic Jan 24, 2020 •

edited

Loading

majagrubic Jan 24, 2020 •

edited

Loading

lizozom Jan 29, 2020

elasticmachine commented Jan 28, 2020

majagrubic commented Jan 28, 2020

lizozom left a comment

lizozom Jan 29, 2020

lizozom Jan 29, 2020

majagrubic commented Jan 29, 2020

majagrubic commented Jan 29, 2020

flash1293 left a comment

flash1293 Jan 30, 2020

flash1293 Jan 30, 2020

kibanamachine commented Jan 30, 2020

majagrubic commented Jan 30, 2020 •

edited

Loading

lizozom commented Jan 30, 2020

[Dashboard] Deangularize Nav Bar #55443

[Dashboard] Deangularize Nav Bar #55443

Conversation

majagrubic commented Jan 21, 2020 • edited Loading

Summary

Checklist

For maintainers

majagrubic Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

majagrubic Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

lizozom Jan 29, 2020

Choose a reason for hiding this comment

elasticmachine commented Jan 28, 2020

majagrubic commented Jan 28, 2020

lizozom left a comment

Choose a reason for hiding this comment

lizozom Jan 29, 2020

Choose a reason for hiding this comment

lizozom Jan 29, 2020

Choose a reason for hiding this comment

majagrubic commented Jan 29, 2020

majagrubic commented Jan 29, 2020

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Jan 30, 2020

Choose a reason for hiding this comment

flash1293 Jan 30, 2020

Choose a reason for hiding this comment

kibanamachine commented Jan 30, 2020

💛 Build succeeded, but was flaky

History

majagrubic commented Jan 30, 2020 • edited Loading

lizozom commented Jan 30, 2020

majagrubic commented Jan 21, 2020 •

edited

Loading

majagrubic Jan 24, 2020 •

edited

Loading

majagrubic Jan 24, 2020 •

edited

Loading

majagrubic commented Jan 30, 2020 •

edited

Loading