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.

Sign up for GitHub

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

Reset chrome fields while switching an app #73064

Merged
merged 11 commits into from
Aug 26, 2020
Merged

Reset chrome fields while switching an app #73064

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4ae2a90
Reset chrome help extension while switching an app
sulemanof Jul 23, 2020
18e5be0
Merge branch 'master' into fix/erase_help_extension
elasticmachine Jul 27, 2020
00b7c84
Merge branch 'master' into fix/erase_help_extension
elasticmachine Jul 29, 2020
2c32c1a
Merge remote-tracking branch 'upstream/master' into fix/erase_help_ex…
sulemanof Jul 30, 2020
485d3ab
Merge branch 'fix/erase_help_extension' of github.com:sulemanof/kiban…
sulemanof Aug 21, 2020
3c49179
Merge remote-tracking branch 'upstream/master' into fix/erase_help_ex…
sulemanof Aug 21, 2020
b75eb98
Reset other chrome fields
sulemanof Aug 21, 2020
2dd1a51
Set docTitle in saved objects app
sulemanof Aug 21, 2020
beb0adb
Merge branch 'master' into fix/erase_help_extension
elasticmachine Aug 24, 2020
002e1da
Merge branch 'master' into fix/erase_help_extension
sulemanof Aug 25, 2020
a3765e7
Add unit tests
sulemanof Aug 25, 2020
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
8 changes: 8 additions & 0 deletions src/core/public/chrome/chrome_service.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,14 @@ export class ChromeService {
const recentlyAccessed = await this.recentlyAccessed.start({ http });
const docTitle = this.docTitle.start({ document: window.document });

// erase chrome fields from a previous app while switching to a next app
application.currentAppId$.subscribe(() => {
helpExtension$.next(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Shouldn't we reset all the Chrome fields then? Badge, Breadcrumbs, etc.
  2. It implicitly enforces calling order for navigation and setHelpExtension. @joshdover should we make this logic explicit? For example, we can move some Chrome params into the Application interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Shouldn't we reset all the Chrome fields then? Badge, Breadcrumbs, etc.
  2. It implicitly enforces calling order for navigation and setHelpExtension. @joshdover should we make this logic explicit? For example, we can move some Chrome params into the Application interface?
  1. Good point, I would also do this on switching an app.
    Especially since I still see couple of places when a page title is not changed after navigating to any app, or such a PR's like Add doc titles to ES UI apps #71045 , where chrome.docTitle.reset() is manually called on application destroy.
    So doing it once after switching to any app would close other issues.
  2. I was also under assumption to set default chrome params explicitly in navigateToApp, but discovered that chrome service depends on application one.
    So, the other implementation way would cause significant code changes.

I'm excited to receive @joshdover opinion! 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Yes, let's go ahead and do those for the chrome items that make sense.
  2. I agree this would be much better. I think it would be best to split that change into two phases where we first introduce the new API, separately migrate each usage of the existing imperative APIs, and then remove the imperative APIs. This could be done as a different issue / PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshdover Should I expand this PR with other chrome items reset, or close it in favor of new API ?

Copy link
Contributor

@mshustov mshustov Jul 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Should I expand this PR with other chrome items reset, or close it in favor of new API ?

We can fix it here quickly and refactor chrome service after. Another complain: #72671

but let's wait for @joshdover opinion

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I agree, let's go ahead and do the quick fix now in this PR and refactor later.

breadcrumbs$.next([]);
badge$.next(undefined);
docTitle.reset();
});

const setIsNavDrawerLocked = (isLocked: boolean) => {
isNavDrawerLocked$.next(isLocked);
localStorage.setItem(IS_LOCKED_KEY, `${isLocked}`);
Expand Down
7 changes: 7 additions & 0 deletions src/plugins/saved_objects_management/public/management_section/mount_section.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import React, { lazy, Suspense } from 'react';
import ReactDOM from 'react-dom';
import { Router, Switch, Route } from 'react-router-dom';
import { I18nProvider } from '@kbn/i18n/react';
import { i18n } from '@kbn/i18n';
import { EuiLoadingSpinner } from '@elastic/eui';
import { CoreSetup } from 'src/core/public';
import { ManagementAppMountParams } from '../../../management/public';
Expand All @@ -36,6 +37,10 @@ interface MountParams {

let allowedObjectTypes: string[] | undefined;

const title = i18n.translate('savedObjectsManagement.objects.savedObjectsTitle', {
defaultMessage: 'Saved Objects',
});

const SavedObjectsEditionPage = lazy(() => import('./saved_objects_edition_page'));
const SavedObjectsTablePage = lazy(() => import('./saved_objects_table_page'));
export const mountManagementSection = async ({
Expand All @@ -49,6 +54,8 @@ export const mountManagementSection = async ({
allowedObjectTypes = await getAllowedTypes(coreStart.http);
}

coreStart.chrome.docTitle.change(title);

const capabilities = coreStart.application.capabilities;

const RedirectToHomeIfUnauthorized: React.FunctionComponent = ({ children }) => {
Expand Down