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

Fix chromeless NP apps not using full page width #54550

Merged
merged 6 commits into from
Jan 14, 2020
Merged
Show file tree
Hide file tree
Changes from 4 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
2 changes: 1 addition & 1 deletion src/core/public/chrome/chrome_service.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ export class ChromeService {
)
)
);
this.isVisible$ = combineLatest(this.appHidden$, this.toggleHidden$).pipe(
this.isVisible$ = combineLatest([this.appHidden$, this.toggleHidden$]).pipe(
Copy link
Contributor

Choose a reason for hiding this comment

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

Did it change any logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, changed it because the previous signature is deprecated.

map(([appHidden, toggleHidden]) => !(appHidden || toggleHidden)),
takeUntil(this.stop$)
);
Expand Down
105 changes: 105 additions & 0 deletions src/core/public/rendering/app_containers.test.tsx
Original file line number Diff line number Diff line change
@@ -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<boolean>(true);

const component = mount(<AppWrapper chromeVisible$={chromeVisible$}>app-content</AppWrapper>);
expect(component.getDOMNode()).toMatchInlineSnapshot(`
<div
class="app-wrapper"
>
app-content
</div>
`);

act(() => chromeVisible$.next(false));
component.update();
expect(component.getDOMNode()).toMatchInlineSnapshot(`
<div
class="app-wrapper hidden-chrome"
>
app-content
</div>
`);

act(() => chromeVisible$.next(true));
component.update();
expect(component.getDOMNode()).toMatchInlineSnapshot(`
<div
class="app-wrapper"
>
app-content
</div>
`);
});
});

describe('AppContainer', () => {
it('adds classes supplied by chrome', () => {
const appClasses$ = new BehaviorSubject<string[]>([]);

const component = mount(<AppContainer classes$={appClasses$}>app-content</AppContainer>);
expect(component.getDOMNode()).toMatchInlineSnapshot(`
<div
class="application"
>
app-content
</div>
`);

act(() => appClasses$.next(['classA', 'classB']));
component.update();
expect(component.getDOMNode()).toMatchInlineSnapshot(`
<div
class="application classA classB"
>
app-content
</div>
`);

act(() => appClasses$.next(['classC']));
component.update();
expect(component.getDOMNode()).toMatchInlineSnapshot(`
<div
class="application classC"
>
app-content
</div>
`);

act(() => appClasses$.next([]));
component.update();
expect(component.getDOMNode()).toMatchInlineSnapshot(`
<div
class="application"
>
app-content
</div>
`);
});
});
37 changes: 37 additions & 0 deletions src/core/public/rendering/app_containers.tsx
Original file line number Diff line number Diff line change
@@ -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 cn from 'classnames';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: other code in the repo uses import classnames/classNames from 'classnames';


export const AppWrapper: React.FunctionComponent<{
chromeVisible$: Observable<boolean>;
}> = ({ chromeVisible$, children }) => {
const visible = useObservable(chromeVisible$);
return <div className={cn('app-wrapper', { 'hidden-chrome': !visible })}>{children}</div>;
};

export const AppContainer: React.FunctionComponent<{
classes$: Observable<string[]>;
}> = ({ classes$, children }) => {
const classes = useObservable(classes$);
return <div className={cn('application', classes)}>{children}</div>;
};
151 changes: 103 additions & 48 deletions src/core/public/rendering/rendering_service.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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: () => <div>Hello application!</div>,
} as InternalApplicationStart;
const chrome = chromeServiceMock.createStartContract();
let application: ReturnType<typeof applicationServiceMock.createInternalStartContract>;
let chrome: ReturnType<typeof chromeServiceMock.createStartContract>;
let overlays: ReturnType<typeof overlayServiceMock.createStartContract>;
let injectedMetadata: ReturnType<typeof injectedMetadataServiceMock.createStartContract>;
let targetDomElement: HTMLDivElement;
let rendering: RenderingService;

beforeEach(() => {
application = applicationServiceMock.createInternalStartContract();
application.getComponent.mockReturnValue(<div>Hello application!</div>);

chrome = chromeServiceMock.createStartContract();
chrome.getHeaderComponent.mockReturnValue(<div>Hello chrome!</div>);
const overlays = overlayServiceMock.createStartContract();

overlays = overlayServiceMock.createStartContract();
overlays.banners.getComponent.mockReturnValue(<div>I&apos;m a banner!</div>);

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(`
<div
class="application"
>
<div>
Hello application!
</div>
</div>
`);
});
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(`
<div
class="application class-name"
>
<div>
Hello application!
</div>
</div>
`);
});

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(`
<div
id="globalBannerList"
>
<div>
I'm a banner!
</div>
</div>
`);
act(() => isVisible$.next(true));
expect(appWrapper.className).toEqual('app-wrapper');
});

it('adds the application classes to the AppContainer', () => {
const applicationClasses$ = new BehaviorSubject<string[]>([]);
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(`
<div
id="globalBannerList"
>
<div>
I'm a banner!
</div>
</div>
`);
});
});

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(`
<div>
<div
Expand All @@ -100,10 +157,8 @@ describe('RenderingService#start', () => {
});

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);
});
});
Expand Down
7 changes: 4 additions & 3 deletions src/core/public/rendering/rendering_service.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -65,12 +66,12 @@ export class RenderingService {
{chromeUi}

{!legacyMode && (
<div className="app-wrapper">
<AppWrapper chromeVisible$={chrome.getIsVisible$()}>
<div className="app-wrapper-panel">
<div id="globalBannerList">{bannerUi}</div>
<div className="application">{appUi}</div>
<AppContainer classes$={chrome.getApplicationClasses$()}>{appUi}</AppContainer>
Comment on lines -71 to +72
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed this in the same PR as we missed it ( the chrome applicationClasses were only properly used when rendering legacy apps), however I did not add functional tests for this, as I'm not sure what the actual usage should be. Tell me if unit test are not enough.

</div>
</div>
</AppWrapper>
)}

{legacyMode && <div ref={legacyRef} />}
Expand Down
Loading