Skip to content

Commit

Permalink
Fix bug in flipped value of application chromeHidden
Browse files Browse the repository at this point in the history
  • Loading branch information
@eliperelman
eliperelman committed Nov 5, 2019
1 parent 9c2b49b commit fea12a3
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 79 deletions.
13 changes: 7 additions & 6 deletions src/core/public/application/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,12 +150,13 @@ export interface AppMountParameters {
* export class MyPlugin implements Plugin {
* setup({ application }) {
* application.register({
* id: 'my-app',
* async mount(context, params) {
* const { renderApp } = await import('./application');
* return renderApp(context, params);
* },
* });
* id: 'my-app',
* async mount(context, params) {
* const { renderApp } = await import('./application');
* return renderApp(context, params);
* },
* });
* }
* }
* ```
*
Expand Down
82 changes: 27 additions & 55 deletions src/core/public/chrome/chrome_service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,34 @@ import { docLinksServiceMock } from '../doc_links/doc_links_service.mock';
import { ChromeService } from './chrome_service';
import { App } from '../application';

class FakeApp implements App {
public title = `${this.id} App`;
public mount = () => () => {};
constructor(public id: string, public chromeHidden?: boolean) {}
}
const store = new Map();
const originalLocalStorage = window.localStorage;

(window as any).localStorage = {
setItem: (key: string, value: string) => store.set(String(key), String(value)),
getItem: (key: string) => store.get(String(key)),
removeItem: (key: string) => store.delete(String(key)),
};

function defaultStartDeps() {
return {
function defaultStartDeps(availableApps?: App[]) {
const deps = {
application: applicationServiceMock.createInternalStartContract(),
docLinks: docLinksServiceMock.createStartContract(),
http: httpServiceMock.createStartContract(),
injectedMetadata: injectedMetadataServiceMock.createStartContract(),
notifications: notificationServiceMock.createStartContract(),
};

if (availableApps) {
deps.application.availableApps = new Map(availableApps.map(app => [app.id, app]));
}

return deps;
}

async function start({
Expand All @@ -65,15 +78,15 @@ async function start({
};
}

function createApp(id: string, chromeHidden?: boolean): [string, App] {
return [id, { id, title: `${id} App`, mount: () => () => {}, chromeHidden }];
}

beforeEach(() => {
store.clear();
window.history.pushState(undefined, '', '#/home?a=b');
});

afterAll(() => {
(window as any).localStorage = originalLocalStorage;
});

describe('start', () => {
it('adds legacy browser warning if browserSupportsCsp is disabled and warnLegacyBrowsers is enabled', async () => {
const { startDeps } = await start({ options: { browserSupportsCsp: false } });
Expand Down Expand Up @@ -168,7 +181,7 @@ describe('start', () => {
`);
});

it('always emits false if embed query string is in hash when set up', async () => {
it('always emits false if embed query string is preset when set up', async () => {
window.history.pushState(undefined, '', '#/home?a=b&embed=true');

const { chrome, service } = await start();
Expand All @@ -193,68 +206,27 @@ describe('start', () => {
});

it('application-specified visibility on mount', async () => {
const startDeps = defaultStartDeps();

// We can fake the application service navigating to the next app by
// adding a fake app to the application's available apps then triggering
// the next ID.
startDeps.application.availableApps = new Map([
createApp('alpha'),
createApp('beta', true),
createApp('gamma', false),
const startDeps = defaultStartDeps([
new FakeApp('alpha'), // An undefined chromeHidden is the same as setting to false.
new FakeApp('beta', true),
new FakeApp('gamma', false),
]);

const { availableApps, currentAppId$ } = startDeps.application;
const { chrome, service } = await start({ startDeps });
const promise = chrome
.getIsVisible$()
.pipe(toArray())
.toPromise();

startDeps.application.currentAppId$.next('alpha');
startDeps.application.currentAppId$.next('beta');
startDeps.application.currentAppId$.next('gamma');
[...availableApps.keys()].forEach(appId => currentAppId$.next(appId));
service.stop();

await expect(promise).resolves.toMatchInlineSnapshot(`
Array [
true,
false,
true,
false,
]
`);
});

it('application-specified visibility has no effect when embed query string is in hash', async () => {
window.history.pushState(undefined, '', '#/home?a=b&embed=true');
const startDeps = defaultStartDeps();

// We can fake the application service navigating to the next app by
// adding a fake app to the application's available apps then triggering
// the next ID.
startDeps.application.availableApps = new Map([
createApp('alpha'),
createApp('beta', true),
createApp('gamma', false),
]);

const { chrome, service } = await start({ startDeps });
const promise = chrome
.getIsVisible$()
.pipe(toArray())
.toPromise();

startDeps.application.currentAppId$.next('alpha');
startDeps.application.currentAppId$.next('beta');
startDeps.application.currentAppId$.next('gamma');
service.stop();

await expect(promise).resolves.toMatchInlineSnapshot(`
Array [
false,
false,
false,
false,
true,
]
`);
});
Expand Down
39 changes: 21 additions & 18 deletions src/core/public/chrome/chrome_service.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

import React from 'react';
import { BehaviorSubject, Observable, ReplaySubject, merge } from 'rxjs';
import { BehaviorSubject, Observable, ReplaySubject, combineLatest, of, merge } from 'rxjs';
import { map, takeUntil } from 'rxjs/operators';
import * as Url from 'url';

Expand Down Expand Up @@ -94,26 +94,20 @@ export class ChromeService {
injectedMetadata,
notifications,
}: StartDeps): Promise<InternalChromeStart> {
const FORCE_HIDDEN = isEmbedParamInHash();

const appTitle$ = new BehaviorSubject<string>('Kibana');
const brand$ = new BehaviorSubject<ChromeBrand>({});
const isCollapsed$ = new BehaviorSubject(!!localStorage.getItem(IS_COLLAPSED_KEY));
const applicationClasses$ = new BehaviorSubject<Set<string>>(new Set());
const helpExtension$ = new BehaviorSubject<ChromeHelpExtension | undefined>(undefined);
const breadcrumbs$ = new BehaviorSubject<ChromeBreadcrumb[]>([]);
const badge$ = new BehaviorSubject<ChromeBadge | undefined>(undefined);

/**
* These observables allow consumers to toggle the chrome visibility via either:
* 1. Using setIsVisible() to trigger the next visibility$
* 1. Using setIsVisible() to trigger the next hide$
* 2. Setting `chromeHidden` when registering an application, which will
* reset the visibility whenever the next application is mounted
* 3. Having embed=true in the query string
*/
const visibility$ = new BehaviorSubject(true);
const isVisible$ = merge(
visibility$,
const chromeHidden$ = new BehaviorSubject(false);
const forceHidden$ = of(isEmbedParamInHash());
const appHidden$ = merge(
// Default the app being hidden to false in case the application service has
// not emitted an app ID yet since we want to trigger combineLatest below
// regardless of having a value yet.
of(false),
application.currentAppId$.pipe(
map(
appId =>
Expand All @@ -122,11 +116,20 @@ export class ChromeService {
!!application.availableApps.get(appId)!.chromeHidden
)
)
).pipe(
map(isVisible => (FORCE_HIDDEN ? false : isVisible)),
);
const isVisible$ = combineLatest(appHidden$, forceHidden$, chromeHidden$).pipe(
map(([appHidden, forceHidden, chromeHidden]) => !(appHidden || forceHidden || chromeHidden)),
takeUntil(this.stop$)
);

const appTitle$ = new BehaviorSubject<string>('Kibana');
const brand$ = new BehaviorSubject<ChromeBrand>({});
const isCollapsed$ = new BehaviorSubject(!!localStorage.getItem(IS_COLLAPSED_KEY));
const applicationClasses$ = new BehaviorSubject<Set<string>>(new Set());
const helpExtension$ = new BehaviorSubject<ChromeHelpExtension | undefined>(undefined);
const breadcrumbs$ = new BehaviorSubject<ChromeBreadcrumb[]>([]);
const badge$ = new BehaviorSubject<ChromeBadge | undefined>(undefined);

const navControls = this.navControls.start();
const navLinks = this.navLinks.start({ application, http });
const recentlyAccessed = await this.recentlyAccessed.start({ http });
Expand Down Expand Up @@ -186,7 +189,7 @@ export class ChromeService {

getIsVisible$: () => isVisible$,

setIsVisible: (visibility: boolean) => visibility$.next(visibility),
setIsVisible: (isVisible: boolean) => chromeHidden$.next(!isVisible),

getIsCollapsed$: () => isCollapsed$.pipe(takeUntil(this.stop$)),

Expand Down

0 comments on commit fea12a3

Please sign in to comment.