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

[Discover] Deangularize navbar in context app #86353

Merged
merged 8 commits into from
Jan 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/plugins/data/public/ui/filter_bar/filter_item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -71,19 +71,27 @@ export function FilterItem(props: Props) {

useEffect(() => {
const index = props.filter.meta.index;
let isSubscribed = true;
Copy link
Member

Choose a reason for hiding this comment

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

yes, no more warning in the console! however we need to take care of rerendering of this component in a follow up PR, which is I think the reason of the former warning.

Copy link
Contributor Author

@majagrubic majagrubic Dec 22, 2020

Choose a reason for hiding this comment

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

I think the reason for the warning was that getIndexPatterns is an async task, so once it's done it tries to update the state of the component, but the component has been unmounted already. The cleanup will run once the component is unmounted, cleanup will unset the flag, which will prevent subsequent updates to the state. This code is not in any way tampering with the lifecycle of the component. The only question here is if there's a more sophisticated way to get this, beyond plain flag setting, but I'll defer this decision to component owners.

if (index) {
getIndexPatterns()
.get(index)
.then((indexPattern) => {
setIndexPatternExists(!!indexPattern);
if (isSubscribed) {
setIndexPatternExists(!!indexPattern);
}
})
.catch(() => {
setIndexPatternExists(false);
if (isSubscribed) {
setIndexPatternExists(false);
}
});
} else {
} else if (isSubscribed) {
// Allow filters without an index pattern and don't validate them.
setIndexPatternExists(true);
}
return () => {
isSubscribed = false;
};
}, [props.filter.meta.index]);

function handleBadgeClick(e: MouseEvent<HTMLInputElement>) {
Expand Down
13 changes: 1 addition & 12 deletions src/plugins/discover/public/application/angular/context_app.html
Original file line number Diff line number Diff line change
@@ -1,15 +1,3 @@
<kbn-top-nav
app-name="'context'"
show-search-bar="true"
show-filter-bar="true"
show-query-bar="false"
show-save-query="false"
show-date-picker="false"
index-patterns="[contextApp.indexPattern]"
use-default-behaviors="true"
>
</kbn-top-nav>

<!-- Context App Legacy -->
<context-app-legacy
filter="contextApp.actions.addFilter"
Expand All @@ -29,4 +17,5 @@
successor-available="contextApp.state.rows.successors.length"
successor-status="contextApp.state.loadingStatus.successors.status"
on-change-successor-count="contextApp.actions.fetchGivenSuccessorRows"
top-nav-menu="contextApp.topNavMenu"
></context-app-legacy>
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,14 @@ getAngularModule().directive('contextApp', function ContextApp() {
});

function ContextAppController($scope, Private) {
const { filterManager, indexPatterns, uiSettings } = getServices();
const { filterManager, indexPatterns, uiSettings, navigation } = getServices();
const queryParameterActions = getQueryParameterActions(filterManager, indexPatterns);
const queryActions = Private(QueryActionsProvider);
this.state = createInitialState(
parseInt(uiSettings.get(CONTEXT_STEP_SETTING), 10),
getFirstSortableField(this.indexPattern, uiSettings.get(CONTEXT_TIE_BREAKER_FIELDS_SETTING))
);
this.topNavMenu = navigation.ui.TopNavMenu;

this.actions = _.mapValues(
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import React from 'react';

export const TopNavMenuMock = () => <div>Hello World</div>;
Copy link
Member

Choose a reason for hiding this comment

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

🌍 : hi @majagrubic

Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { DocTableLegacy } from '../../angular/doc_table/create_doc_table_react';
import { findTestSubject } from '@elastic/eui/lib/test';
import { ActionBar } from '../../angular/context/components/action_bar/action_bar';
import { ContextErrorMessage } from '../context_error_message';
import { TopNavMenuMock } from './__mocks__/top_nav_menu';

describe('ContextAppLegacy test', () => {
const hit = {
Expand Down Expand Up @@ -64,6 +65,17 @@ describe('ContextAppLegacy test', () => {
onChangeSuccessorCount: jest.fn(),
predecessorStatus: 'loaded',
successorStatus: 'loaded',
topNavMenu: TopNavMenuMock,
};
const topNavProps = {
appName: 'context',
showSearchBar: true,
showQueryBar: false,
showFilterBar: true,
showSaveQuery: false,
showDatePicker: false,
indexPatterns: [indexPattern],
useDefaultBehaviors: true,
};

it('renders correctly', () => {
Expand All @@ -72,6 +84,9 @@ describe('ContextAppLegacy test', () => {
const loadingIndicator = findTestSubject(component, 'contextApp_loadingIndicator');
expect(loadingIndicator.length).toBe(0);
expect(component.find(ActionBar).length).toBe(2);
const topNavMenu = component.find(TopNavMenuMock);
expect(topNavMenu.length).toBe(1);
expect(topNavMenu.props()).toStrictEqual(topNavProps);
});

it('renders loading indicator', () => {
Expand All @@ -82,6 +97,7 @@ describe('ContextAppLegacy test', () => {
const loadingIndicator = findTestSubject(component, 'contextApp_loadingIndicator');
expect(loadingIndicator.length).toBe(1);
expect(component.find(ActionBar).length).toBe(2);
expect(component.find(TopNavMenuMock).length).toBe(1);
});

it('renders error message', () => {
Expand All @@ -90,6 +106,7 @@ describe('ContextAppLegacy test', () => {
props.reason = 'something went wrong';
const component = mountWithIntl(<ContextAppLegacy {...props} />);
expect(component.find(DocTableLegacy).length).toBe(0);
expect(component.find(TopNavMenuMock).length).toBe(0);
const errorMessage = component.find(ContextErrorMessage);
expect(errorMessage.length).toBe(1);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@ import {
import { IIndexPattern, IndexPatternField } from '../../../../../data/common/index_patterns';
import { LOADING_STATUS } from './constants';
import { ActionBar, ActionBarProps } from '../../angular/context/components/action_bar/action_bar';
import { TopNavMenuProps } from '../../../../../navigation/public';

export interface ContextAppProps {
topNavMenu: React.ComponentType<TopNavMenuProps>;
columns: string[];
hits: Array<Record<string, unknown>>;
indexPattern: IIndexPattern;
Expand Down Expand Up @@ -96,6 +98,20 @@ export function ContextAppLegacy(renderProps: ContextAppProps) {
} as DocTableLegacyProps;
};

const TopNavMenu = renderProps.topNavMenu;
const getNavBarProps = () => {
return {
appName: 'context',
showSearchBar: true,
showQueryBar: false,
showFilterBar: true,
showSaveQuery: false,
showDatePicker: false,
indexPatterns: [renderProps.indexPattern],
useDefaultBehaviors: true,
};
};

const loadingFeedback = () => {
if (status === LOADING_STATUS.UNINITIALIZED || status === LOADING_STATUS.LOADING) {
return (
Expand All @@ -112,20 +128,23 @@ export function ContextAppLegacy(renderProps: ContextAppProps) {
{isFailed ? (
<ContextErrorMessage status={status} reason={renderProps.reason} />
) : (
<EuiPage>
<EuiPageContent paddingSize="s" className="dscCxtAppContent">
<ActionBar {...actionBarProps(PREDECESSOR_TYPE)} />
{loadingFeedback()}
<EuiHorizontalRule margin="xs" />
{isLoaded ? (
<div className="discover-table">
<DocTableLegacy {...docTableProps()} />
</div>
) : null}
<EuiHorizontalRule margin="xs" />
<ActionBar {...actionBarProps(SUCCESSOR_TYPE)} />
</EuiPageContent>
</EuiPage>
<div>
<TopNavMenu {...getNavBarProps()} />
<EuiPage>
<EuiPageContent paddingSize="s" className="dscCxtAppContent">
<ActionBar {...actionBarProps(PREDECESSOR_TYPE)} />
{loadingFeedback()}
<EuiHorizontalRule margin="xs" />
{isLoaded ? (
<div className="discover-table">
<DocTableLegacy {...docTableProps()} />
</div>
) : null}
<EuiHorizontalRule margin="xs" />
<ActionBar {...actionBarProps(SUCCESSOR_TYPE)} />
</EuiPageContent>
</EuiPage>
</div>
)}
</I18nProvider>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,6 @@ export function createContextAppLegacy(reactDirective: any) {
['successorAvailable', { watchDepth: 'reference' }],
['successorStatus', { watchDepth: 'reference' }],
['onChangeSuccessorCount', { watchDepth: 'reference' }],
['topNavMenu', { watchDepth: 'reference' }],
]);
}
11 changes: 0 additions & 11 deletions src/plugins/discover/public/get_inner_angular.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,6 @@ import {
PromiseServiceCreator,
registerListenEventListener,
watchMultiDecorator,
createTopNavDirective,
createTopNavHelper,
} from '../../kibana_legacy/public';
import { DiscoverStartPlugins } from './plugin';
import { getScopedHistory } from './kibana_services';
Expand Down Expand Up @@ -98,7 +96,6 @@ export function initializeInnerAngularModule(
createLocalI18nModule();
createLocalPrivateModule();
createLocalPromiseModule();
createLocalTopNavModule(navigation);
createLocalStorageModule();
createPagerFactoryModule();
createDocTableModule();
Expand Down Expand Up @@ -131,7 +128,6 @@ export function initializeInnerAngularModule(
'discoverI18n',
'discoverPrivate',
'discoverPromise',
'discoverTopNav',
'discoverLocalStorageProvider',
'discoverDocTable',
'discoverPagerFactory',
Expand All @@ -151,13 +147,6 @@ function createLocalPrivateModule() {
angular.module('discoverPrivate', []).provider('Private', PrivateProvider);
}

function createLocalTopNavModule(navigation: NavigationStart) {
angular
.module('discoverTopNav', ['react'])
.directive('kbnTopNav', createTopNavDirective)
.directive('kbnTopNavHelper', createTopNavHelper(navigation.ui));
}

function createLocalI18nModule() {
angular
.module('discoverI18n', [])
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/navigation/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export function plugin(initializerContext: PluginInitializerContext) {
return new NavigationPublicPlugin(initializerContext);
}

export { TopNavMenuData, TopNavMenu } from './top_nav_menu';
export { TopNavMenuData, TopNavMenu, TopNavMenuProps } from './top_nav_menu';

export { NavigationPublicPluginSetup, NavigationPublicPluginStart } from './types';

Expand Down