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

[core/public/chrome] migrate controls, theme, and visibility apis #22987

Merged
merged 10 commits into from
Oct 19, 2018
215 changes: 215 additions & 0 deletions src/core/public/chrome/chrome_service.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
/*
* 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 * as Rx from 'rxjs';
import { toArray } from 'rxjs/operators';

const store = new Map();
(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)),
};

import { ChromeService } from './chrome_service';

beforeEach(() => {
store.clear();
});

describe('start', () => {
describe('brand', () => {
it('updates/emits the brand as it changes', async () => {
const service = new ChromeService();
const start = service.start();
const promise = start
.getBrand$()
.pipe(toArray())
.toPromise();

start.setBrand({
logo: 'big logo',
smallLogo: 'not so big logo',
});
start.setBrand({
logo: 'big logo without small logo',
});
service.stop();

await expect(promise).resolves.toMatchInlineSnapshot(`
Copy link
Contributor

@cjcenizal cjcenizal Sep 24, 2018

Choose a reason for hiding this comment

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

Is there an advantage to using toMatchInlineSnapshot over toEqual? Personally I would find the latter more readable:

      await expect(promise).resolves.toEqual([
        {},
        {
          logo: "big logo",
          smallLogo: "not so big logo",
        },
        {
          logo: "big logo without small logo",
          smallLogo: undefined,
        },
      ]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly maintenance, being able to update the snapshots with -u and automatically update them with --watch are pretty huge benefits to me

Array [
Object {},
Object {
"logo": "big logo",
"smallLogo": "not so big logo",
},
Object {
"logo": "big logo without small logo",
"smallLogo": undefined,
},
]
`);
});
});

describe('visibility', () => {
it('updates/emits the visibility', async () => {
const service = new ChromeService();
const start = service.start();
const promise = start
.getIsVisible$()
.pipe(toArray())
.toPromise();

start.setIsVisible(true);
start.setIsVisible(false);
start.setIsVisible(true);
service.stop();

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

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

const service = new ChromeService();
const start = service.start();
const promise = start
.getIsVisible$()
.pipe(toArray())
.toPromise();

start.setIsVisible(true);
start.setIsVisible(false);
start.setIsVisible(true);
service.stop();

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

describe('is collapsed', () => {
it('updates/emits isCollapsed', async () => {
const service = new ChromeService();
const start = service.start();
const promise = start
.getIsCollapsed$()
.pipe(toArray())
.toPromise();

start.setIsCollapsed(true);
start.setIsCollapsed(false);
start.setIsCollapsed(true);
service.stop();

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

it('only stores true in localStorage', async () => {
const service = new ChromeService();
const start = service.start();

start.setIsCollapsed(true);
expect(store.size).toBe(1);

start.setIsCollapsed(false);
expect(store.size).toBe(0);
});
});

describe('application classes', () => {
it('updates/emits the application classes', async () => {
const service = new ChromeService();
const start = service.start();
const promise = start
.getApplicationClasses$()
.pipe(toArray())
.toPromise();

start.addApplicationClass('foo');
start.addApplicationClass('bar');
start.addApplicationClass('baz');
spalger marked this conversation as resolved.
Show resolved Hide resolved
start.removeApplicationClass('bar');
start.removeApplicationClass('foo');
service.stop();

await expect(promise).resolves.toMatchInlineSnapshot(`
Array [
Array [],
Array [
"foo",
],
Array [
"foo",
"bar",
],
Array [
"foo",
"bar",
"baz",
],
Array [
"foo",
"baz",
],
Array [
"baz",
],
]
`);
});
});
});

describe('stop', () => {
it('completes applicationClass$, isCollapsed$, isVisible$, and brand$ observables', async () => {
const service = new ChromeService();
const start = service.start();
const promise = Rx.combineLatest(
start.getBrand$(),
start.getApplicationClasses$(),
start.getIsCollapsed$(),
start.getIsVisible$()
).toPromise();

service.stop();
await promise;
});
});
145 changes: 145 additions & 0 deletions src/core/public/chrome/chrome_service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
/*
* 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 * as Url from 'url';

import * as Rx from 'rxjs';
import { map, takeUntil } from 'rxjs/operators';

const IS_COLLAPSED_KEY = 'core.chrome.isCollapsed';

function isEmbedParamInHash() {
const { query } = Url.parse(String(window.location.hash).slice(1), true);
return Boolean(query.embed);
}

export interface Brand {
logo?: string;
smallLogo?: string;
}

export class ChromeService {
private readonly stop$ = new Rx.ReplaySubject(1);
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we use just Rx.Subject() here? If not, maybe leave comment here why or better add a test that tests the behavior we want to achieve that only ReplaySubject can help with (or if put simply fails with Subject and passes with ReplaySubject).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous strategy of using subjects that get complete()'d in stop guarantees that subscriptions after stop() will be completed immediately, in order to replicate that behavior here we need a ReplaySubject that will trigger takeUntil() to stop as soon as it is subscribed. Added a test for this in 456e11e


public start() {
const FORCE_HIDDEN = isEmbedParamInHash();

const brand$ = new Rx.BehaviorSubject<Brand>({});
const isVisible$ = new Rx.BehaviorSubject(true);
const isCollapsed$ = new Rx.BehaviorSubject(!!localStorage.getItem(IS_COLLAPSED_KEY));
const applicationClasses$ = new Rx.BehaviorSubject<Set<string>>(new Set());

return {
/**
* Set the brand configuration. Normally the `logo` property will be rendered as the
* CSS background for the home link in the chrome navigation, but when the page is renderd
spalger marked this conversation as resolved.
Show resolved Hide resolved
* in a small window the `smallLogo` will be used and rendered at about 45px wide.
*
* example:
*
* chrome.setBrand({
* logo: 'url(/plugins/app/logo.png) center no-repeat'
* smallLogo: 'url(/plugins/app/logo-small.png) center no-repeat'
* })
*
*/
setBrand: (brand: Brand) => {
brand$.next(
Object.freeze({
logo: brand.logo,
smallLogo: brand.smallLogo,
})
);
},

/**
* Get an observable of the current brand information.
*/
getBrand$: () => brand$.pipe(takeUntil(this.stop$)),

/**
* Set the temporary visibility for the chrome. This does nothing if the chrome is hidden
* by default and should be used to hide the chrome for things like full-screen modes
* with an exit button.
*/
setIsVisible: (visibility: boolean) => {
isVisible$.next(visibility);
},

/**
* Get an observable of the current visiblity state of the chrome.
spalger marked this conversation as resolved.
Show resolved Hide resolved
*/
getIsVisible$: () =>
Copy link
Member

Choose a reason for hiding this comment

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

question: mostly to widen my RxJS horizons :) What issues we'd have if we use something like this instead?

FORCE_HIDDEN ? of(false) : isVisible$.pipe(takeUntil(this.stop$))

With the current implementation if FORCE_HIDDEN === true then next on getIsVisible$ will always be called with false whenever setIsVisible is called and hence any external code that relies on this observable will be called as well .

Kind of related question, should we next in setIsVisible and setIsCollapsed if value hasn't changed or it should be responsibility of the consumer to use or not distinct*?

Not a big deal, just curious what you think.

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 think something like FORCE_HIDDEN ? of(false) : ... would be fine, but I'm kind of concerned about the difference in lifecycle between the two return values, it probably wouldn't be a big deal, and since it really can't change it is "complete", but I'd have to think through how that might impact consumers. As for deduping here rather than in consumers, I originally had more dedupe logic in here, but felt like it didn't really add anything to do the extra work of deduping notifications. It might turn out that it's a lot nicer to consume an API that properly dedupes notifications, but I'm not sure about that... So I left it out for now.

isVisible$.pipe(
map(visibility => (FORCE_HIDDEN ? false : visibility)),
takeUntil(this.stop$)
),

/**
* Set the collapsed state of the chrome navigation.
*/
setIsCollapsed: (isCollapsed: boolean) => {
isCollapsed$.next(isCollapsed);
if (isCollapsed) {
localStorage.setItem(IS_COLLAPSED_KEY, 'true');
} else {
localStorage.removeItem(IS_COLLAPSED_KEY);
}
},

/**
* Get an observable of the current collapsed state of the chrome.
*/
getIsCollapsed$: () => isCollapsed$.pipe(takeUntil(this.stop$)),

/**
* Add a className that should be set on the application container.
*/
addApplicationClass: (className: string) => {
const update = new Set([...applicationClasses$.getValue()]);
update.add(className);
applicationClasses$.next(update);
},

/**
* Remove a className added with `addApplicationClass()`. If className is unknown it is ignored.
*/
removeApplicationClass: (className: string) => {
const update = new Set([...applicationClasses$.getValue()]);
update.delete(className);
applicationClasses$.next(update);
},

/**
* Get the current set of classNames that will be set on the application container.
*/
getApplicationClasses$: () =>
applicationClasses$.pipe(
map(set => [...set]),
takeUntil(this.stop$)
),
};
}

public stop() {
this.stop$.next();
}
}

export type ChromeStartContract = ReturnType<ChromeService['start']>;
20 changes: 20 additions & 0 deletions src/core/public/chrome/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* 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.
*/

export { ChromeService, ChromeStartContract, Brand } from './chrome_service';
Loading