From 17480540bc5f54f8d1865a99dc622dd552ebbe2d Mon Sep 17 00:00:00 2001 From: Pierre Gayvallet Date: Tue, 14 Jan 2020 09:57:24 +0100 Subject: [PATCH] Fix chromeless NP apps not using full page width (#54550) (#54683) * add missing conditional classes on app-wrapper and application containers * update snapshot * refactor and add unit tests for service * typo * use consistent classNames naming --- src/core/public/chrome/chrome_service.tsx | 2 +- .../public/rendering/app_containers.test.tsx | 105 ++++++++++++ src/core/public/rendering/app_containers.tsx | 37 +++++ .../rendering/rendering_service.test.tsx | 151 ++++++++++++------ .../public/rendering/rendering_service.tsx | 7 +- .../test_suites/core_plugins/applications.ts | 14 ++ 6 files changed, 264 insertions(+), 52 deletions(-) create mode 100644 src/core/public/rendering/app_containers.test.tsx create mode 100644 src/core/public/rendering/app_containers.tsx diff --git a/src/core/public/chrome/chrome_service.tsx b/src/core/public/chrome/chrome_service.tsx index a674b49a8e1344..09ea1afe35766c 100644 --- a/src/core/public/chrome/chrome_service.tsx +++ b/src/core/public/chrome/chrome_service.tsx @@ -127,7 +127,7 @@ export class ChromeService { ) ) ); - this.isVisible$ = combineLatest(this.appHidden$, this.toggleHidden$).pipe( + this.isVisible$ = combineLatest([this.appHidden$, this.toggleHidden$]).pipe( map(([appHidden, toggleHidden]) => !(appHidden || toggleHidden)), takeUntil(this.stop$) ); diff --git a/src/core/public/rendering/app_containers.test.tsx b/src/core/public/rendering/app_containers.test.tsx new file mode 100644 index 00000000000000..746e37b1214d92 --- /dev/null +++ b/src/core/public/rendering/app_containers.test.tsx @@ -0,0 +1,105 @@ +/* + * 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 { BehaviorSubject } from 'rxjs'; +import { act } from 'react-dom/test-utils'; +import { mount } from 'enzyme'; +import React from 'react'; + +import { AppWrapper, AppContainer } from './app_containers'; + +describe('AppWrapper', () => { + it('toggles the `hidden-chrome` class depending on the chrome visibility state', () => { + const chromeVisible$ = new BehaviorSubject(true); + + const component = mount(app-content); + expect(component.getDOMNode()).toMatchInlineSnapshot(` +
+ app-content +
+ `); + + act(() => chromeVisible$.next(false)); + component.update(); + expect(component.getDOMNode()).toMatchInlineSnapshot(` +
+ app-content +
+ `); + + act(() => chromeVisible$.next(true)); + component.update(); + expect(component.getDOMNode()).toMatchInlineSnapshot(` +
+ app-content +
+ `); + }); +}); + +describe('AppContainer', () => { + it('adds classes supplied by chrome', () => { + const appClasses$ = new BehaviorSubject([]); + + const component = mount(app-content); + expect(component.getDOMNode()).toMatchInlineSnapshot(` +
+ app-content +
+ `); + + act(() => appClasses$.next(['classA', 'classB'])); + component.update(); + expect(component.getDOMNode()).toMatchInlineSnapshot(` +
+ app-content +
+ `); + + act(() => appClasses$.next(['classC'])); + component.update(); + expect(component.getDOMNode()).toMatchInlineSnapshot(` +
+ app-content +
+ `); + + act(() => appClasses$.next([])); + component.update(); + expect(component.getDOMNode()).toMatchInlineSnapshot(` +
+ app-content +
+ `); + }); +}); diff --git a/src/core/public/rendering/app_containers.tsx b/src/core/public/rendering/app_containers.tsx new file mode 100644 index 00000000000000..72faaeac588bef --- /dev/null +++ b/src/core/public/rendering/app_containers.tsx @@ -0,0 +1,37 @@ +/* + * 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'; +import { Observable } from 'rxjs'; +import useObservable from 'react-use/lib/useObservable'; +import classNames from 'classnames'; + +export const AppWrapper: React.FunctionComponent<{ + chromeVisible$: Observable; +}> = ({ chromeVisible$, children }) => { + const visible = useObservable(chromeVisible$); + return
{children}
; +}; + +export const AppContainer: React.FunctionComponent<{ + classes$: Observable; +}> = ({ classes$, children }) => { + const classes = useObservable(classes$); + return
{children}
; +}; diff --git a/src/core/public/rendering/rendering_service.test.tsx b/src/core/public/rendering/rendering_service.test.tsx index ed835574a32f9c..437a602a3d4473 100644 --- a/src/core/public/rendering/rendering_service.test.tsx +++ b/src/core/public/rendering/rendering_service.test.tsx @@ -18,72 +18,129 @@ */ import React from 'react'; +import { act } from 'react-dom/test-utils'; -import { chromeServiceMock } from '../chrome/chrome_service.mock'; import { RenderingService } from './rendering_service'; -import { InternalApplicationStart } from '../application'; +import { applicationServiceMock } from '../application/application_service.mock'; +import { chromeServiceMock } from '../chrome/chrome_service.mock'; import { injectedMetadataServiceMock } from '../injected_metadata/injected_metadata_service.mock'; import { overlayServiceMock } from '../overlays/overlay_service.mock'; +import { BehaviorSubject } from 'rxjs'; describe('RenderingService#start', () => { - const getService = ({ legacyMode = false }: { legacyMode?: boolean } = {}) => { - const rendering = new RenderingService(); - const application = { - getComponent: () =>
Hello application!
, - } as InternalApplicationStart; - const chrome = chromeServiceMock.createStartContract(); + let application: ReturnType; + let chrome: ReturnType; + let overlays: ReturnType; + let injectedMetadata: ReturnType; + let targetDomElement: HTMLDivElement; + let rendering: RenderingService; + + beforeEach(() => { + application = applicationServiceMock.createInternalStartContract(); + application.getComponent.mockReturnValue(
Hello application!
); + + chrome = chromeServiceMock.createStartContract(); chrome.getHeaderComponent.mockReturnValue(
Hello chrome!
); - const overlays = overlayServiceMock.createStartContract(); + + overlays = overlayServiceMock.createStartContract(); overlays.banners.getComponent.mockReturnValue(
I'm a banner!
); - const injectedMetadata = injectedMetadataServiceMock.createStartContract(); - injectedMetadata.getLegacyMode.mockReturnValue(legacyMode); - const targetDomElement = document.createElement('div'); - const start = rendering.start({ + injectedMetadata = injectedMetadataServiceMock.createStartContract(); + + targetDomElement = document.createElement('div'); + + rendering = new RenderingService(); + }); + + const startService = () => { + return rendering.start({ application, chrome, injectedMetadata, overlays, targetDomElement, }); - return { start, targetDomElement }; }; - it('renders application service into provided DOM element', () => { - const { targetDomElement } = getService(); - expect(targetDomElement.querySelector('div.application')).toMatchInlineSnapshot(` -
-
- Hello application! -
-
- `); - }); + describe('standard mode', () => { + beforeEach(() => { + injectedMetadata.getLegacyMode.mockReturnValue(false); + }); - it('contains wrapper divs', () => { - const { targetDomElement } = getService(); - expect(targetDomElement.querySelector('div.app-wrapper')).toBeDefined(); - expect(targetDomElement.querySelector('div.app-wrapper-pannel')).toBeDefined(); - }); + it('renders application service into provided DOM element', () => { + startService(); + expect(targetDomElement.querySelector('div.application')).toMatchInlineSnapshot(` +
+
+ Hello application! +
+
+ `); + }); + + it('adds the `chrome-hidden` class to the AppWrapper when chrome is hidden', () => { + const isVisible$ = new BehaviorSubject(true); + chrome.getIsVisible$.mockReturnValue(isVisible$); + startService(); + + const appWrapper = targetDomElement.querySelector('div.app-wrapper')!; + expect(appWrapper.className).toEqual('app-wrapper'); + + act(() => isVisible$.next(false)); + expect(appWrapper.className).toEqual('app-wrapper hidden-chrome'); - it('renders the banner UI', () => { - const { targetDomElement } = getService(); - expect(targetDomElement.querySelector('#globalBannerList')).toMatchInlineSnapshot(` -
-
- I'm a banner! -
-
- `); + act(() => isVisible$.next(true)); + expect(appWrapper.className).toEqual('app-wrapper'); + }); + + it('adds the application classes to the AppContainer', () => { + const applicationClasses$ = new BehaviorSubject([]); + chrome.getApplicationClasses$.mockReturnValue(applicationClasses$); + startService(); + + const appContainer = targetDomElement.querySelector('div.application')!; + expect(appContainer.className).toEqual('application'); + + act(() => applicationClasses$.next(['classA', 'classB'])); + expect(appContainer.className).toEqual('application classA classB'); + + act(() => applicationClasses$.next(['classC'])); + expect(appContainer.className).toEqual('application classC'); + + act(() => applicationClasses$.next([])); + expect(appContainer.className).toEqual('application'); + }); + + it('contains wrapper divs', () => { + startService(); + expect(targetDomElement.querySelector('div.app-wrapper')).toBeDefined(); + expect(targetDomElement.querySelector('div.app-wrapper-pannel')).toBeDefined(); + }); + + it('renders the banner UI', () => { + startService(); + expect(targetDomElement.querySelector('#globalBannerList')).toMatchInlineSnapshot(` +
+
+ I'm a banner! +
+
+ `); + }); }); - describe('legacyMode', () => { + describe('legacy mode', () => { + beforeEach(() => { + injectedMetadata.getLegacyMode.mockReturnValue(true); + }); + it('renders into provided DOM element', () => { - const { targetDomElement } = getService({ legacyMode: true }); + startService(); + expect(targetDomElement).toMatchInlineSnapshot(`
{ }); it('returns a div for the legacy service to render into', () => { - const { - start: { legacyTargetDomElement }, - targetDomElement, - } = getService({ legacyMode: true }); + const { legacyTargetDomElement } = startService(); + expect(targetDomElement.contains(legacyTargetDomElement!)).toBe(true); }); }); diff --git a/src/core/public/rendering/rendering_service.tsx b/src/core/public/rendering/rendering_service.tsx index 7a747faa2673f5..58b8c1921e333b 100644 --- a/src/core/public/rendering/rendering_service.tsx +++ b/src/core/public/rendering/rendering_service.tsx @@ -25,6 +25,7 @@ import { InternalChromeStart } from '../chrome'; import { InternalApplicationStart } from '../application'; import { InjectedMetadataStart } from '../injected_metadata'; import { OverlayStart } from '../overlays'; +import { AppWrapper, AppContainer } from './app_containers'; interface StartDeps { application: InternalApplicationStart; @@ -65,12 +66,12 @@ export class RenderingService { {chromeUi} {!legacyMode && ( -
+
{bannerUi}
-
{appUi}
+ {appUi}
-
+ )} {legacyMode &&
} diff --git a/test/plugin_functional/test_suites/core_plugins/applications.ts b/test/plugin_functional/test_suites/core_plugins/applications.ts index a3c9d9d63e3534..231458fad155b3 100644 --- a/test/plugin_functional/test_suites/core_plugins/applications.ts +++ b/test/plugin_functional/test_suites/core_plugins/applications.ts @@ -27,12 +27,18 @@ export default function({ getService, getPageObjects }: PluginFunctionalProvider const browser = getService('browser'); const appsMenu = getService('appsMenu'); const testSubjects = getService('testSubjects'); + const find = getService('find'); const loadingScreenNotShown = async () => expect(await testSubjects.exists('kbnLoadingMessage')).to.be(false); const loadingScreenShown = () => testSubjects.existOrFail('kbnLoadingMessage'); + const getAppWrapperWidth = async () => { + const wrapper = await find.byClassName('app-wrapper'); + return (await wrapper.getSize()).width; + }; + const getKibanaUrl = (pathname?: string, search?: string) => url.format({ protocol: 'http:', @@ -99,12 +105,20 @@ export default function({ getService, getPageObjects }: PluginFunctionalProvider await PageObjects.common.navigateToApp('chromeless'); await loadingScreenNotShown(); expect(await testSubjects.exists('headerGlobalNav')).to.be(false); + + const wrapperWidth = await getAppWrapperWidth(); + const windowWidth = (await browser.getWindowSize()).width; + expect(wrapperWidth).to.eql(windowWidth); }); it('navigating away from chromeless application shows chrome', async () => { await PageObjects.common.navigateToApp('foo'); await loadingScreenNotShown(); expect(await testSubjects.exists('headerGlobalNav')).to.be(true); + + const wrapperWidth = await getAppWrapperWidth(); + const windowWidth = (await browser.getWindowSize()).width; + expect(wrapperWidth).to.be.below(windowWidth); }); it.skip('can navigate from NP apps to legacy apps', async () => {