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

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Sep 13, 2018

Another part of #19992, moved over the ui/chrome apis that control the theme (dark mode, custom home logo), visibility (embed mode), and controls (collapsed state) to a core.chrome service. All of the original APIs are unchanged. This primary things to test are the collapse/expand button in the navigation, full screen mode in dashboard, the format of PDFs, and dashboard light/dark mode switching.

@spalger spalger added the WIP Work in progress label Sep 13, 2018
@elasticmachine

This comment has been minimized.

@spalger spalger force-pushed the migrate-new-platform/chrome-apis branch 2 times, most recently from 03e2601 to 7c3a2d7 Compare September 13, 2018 18:14
@elasticmachine

This comment has been minimized.

@spalger spalger force-pushed the migrate-new-platform/chrome-apis branch from 8d2299b to 3486219 Compare September 13, 2018 20:34
@elasticmachine

This comment has been minimized.

@spalger spalger force-pushed the migrate-new-platform/chrome-apis branch from 3486219 to 1e41a93 Compare September 13, 2018 22:53
@elasticmachine

This comment has been minimized.

@spalger spalger force-pushed the migrate-new-platform/chrome-apis branch from 1e41a93 to 5e0cf9f Compare September 14, 2018 20:37
@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@spalger spalger force-pushed the migrate-new-platform/chrome-apis branch 2 times, most recently from cb78009 to ed3a9a1 Compare September 15, 2018 07:22
@elasticmachine

This comment has been minimized.

@spalger spalger force-pushed the migrate-new-platform/chrome-apis branch 3 times, most recently from dcf21aa to b143fbb Compare September 15, 2018 22:16
@spalger spalger changed the title [core/public/chrome] migrate theme and controls apis [core/public/chrome] migrate controls, theme, and visibility apis Sep 15, 2018
@spalger spalger force-pushed the migrate-new-platform/chrome-apis branch 2 times, most recently from 3b6912a to 730ecdb Compare September 15, 2018 23:20
@elasticmachine

This comment has been minimized.

@spalger spalger force-pushed the migrate-new-platform/chrome-apis branch from 730ecdb to 6349a9a Compare September 16, 2018 00:56
@elasticmachine

This comment has been minimized.

@spalger spalger force-pushed the migrate-new-platform/chrome-apis branch from 6349a9a to d6ecd65 Compare September 16, 2018 08:49
@elasticmachine

This comment has been minimized.

@spalger spalger removed the WIP Work in progress label Sep 17, 2018
@spalger spalger force-pushed the migrate-new-platform/chrome-apis branch from d6ecd65 to dd151d6 Compare September 17, 2018 14:55
@elasticmachine

This comment has been minimized.

@spalger spalger requested a review from azasypkin September 17, 2018 16:34
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@azasypkin
Copy link
Member

ack: will get to this PR tomorrow morning.

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Code LGTM, thanks for adding tests! Testing locally at the moment.

src/core/public/core_system.ts Show resolved Hide resolved
src/core/public/chrome/chrome_service.ts Outdated Show resolved Hide resolved
src/core/public/chrome/chrome_service.ts Outdated Show resolved Hide resolved
/**
* Get an observable of the current visiblity state of the chrome.
*/
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.

src/ui/public/chrome/api/controls.ts Outdated Show resolved Hide resolved
src/ui/public/chrome/api/controls.ts Outdated Show resolved Hide resolved
src/ui/public/chrome/api/theme.test.ts Outdated Show resolved Hide resolved
src/ui/public/chrome/api/theme.test.ts Show resolved Hide resolved
src/core/public/chrome/chrome_service.test.ts Show resolved Hide resolved
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Tested Kibana in embedded and full screen modes, generated PDF, etc. - everything looked good!

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

});
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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spalger spalger merged commit 208bd51 into elastic:master Oct 19, 2018
spalger pushed a commit to spalger/kibana that referenced this pull request Oct 19, 2018
…astic#22987)

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

* [core/public] stop uiSettings service

* [core/public/chrome] test that observables stop immedaiately after stop()

* fix typos

* [core/public/legacyPlatform] test globalNavState init

* [ui/chrome] don't pass extra params

* [core/public/chrome] test for dedupe-handling

* [ui/chrome/theme] test with different values for logo and smallLogo
spalger pushed a commit that referenced this pull request Oct 20, 2018
…2987) (#24308)

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

* [core/public] stop uiSettings service

* [core/public/chrome] test that observables stop immedaiately after stop()

* fix typos

* [core/public/legacyPlatform] test globalNavState init

* [ui/chrome] don't pass extra params

* [core/public/chrome] test for dedupe-handling

* [ui/chrome/theme] test with different values for logo and smallLogo
@spalger
Copy link
Contributor Author

spalger commented Oct 20, 2018

6.5/6.x: e33cba3

@spalger spalger deleted the migrate-new-platform/chrome-apis branch October 20, 2018 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants