Migrate ui/doc_title to New platform by pgayvallet · Pull Request #48121 · elastic/kibana

pgayvallet · 2019-10-14T15:54:34Z

Summary

Migrate ui/doc_title to new platform:

Add a DocTitle sub-service to ChromeService
Bridge the legacy ui/doc_title to use the NP service

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

Dev Docs

Migrate chrome.docTitle to new platform. Plugins can now change the page title using this API.

coreStart.docTitle.change('My Title');

elasticmachine · 2019-10-14T15:54:36Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet · 2019-10-14T15:57:17Z

src/core/public/chrome/doc_title/doc_title_service.ts

We don't have any other services with that kind of legacy API, however this was needed to keep the old docTitle karma test working, as they were using 'private' APIs to manually change to baseTitle of the old module. Please tell me if these is a better approach

What I've been doing with the legacy tests is just making them dumb unit tests that ensure the NP API was called by using a mock. That way we don't have to do things like this, which should save us time when we start removing the legacy code.

I think this is a good way to bridge the old tests.

@joshdover issue I had here is with the tests in src/legacy/ui/public/doc_title/__tests__/doc_title.js that were basically injected in the old docTitle module a different base name. Could find another way than than to keep the test. Do you see another approach ? Or maybe should I just remove the legacy test as we now got that covered in the NP module tests instead ?

I'm fine with removing this test completely.

pgayvallet · 2019-10-14T15:58:23Z

src/core/public/chrome/doc_title/doc_title_service.ts

This is only a nice to have, but it felt like this could ease the developer way to call the API. If we prefer something more rigid, we can stick to ChromeDocTitleEntry instead

There's currently only one plugin that uses excludeBase=true, although we want to keep BWC in most cases in my opinion we should remove this flag and make it consistent across Kibana. I've asked the ingest team for feedback if they rely on this behaviour.

kibana/x-pack/legacy/plugins/monitoring/public/services/title.js

Lines 18 to 22 in 7ac8e4d

docTitle.change(

i18n.translate('xpack.monitoring.stackMonitoringDocTitle', {

defaultMessage: 'Stack Monitoring {clusterName} {suffix}',

values: { clusterName, suffix }

}), true);

Now that I think more about this... this service does just two things, if you give it parts it joins them with - and it adds the baseTitle. It's so small it's almost annoying :-P but the value it provides is consistency for all of Kibana. If a plugin really needs to break this consistency it feels like they might as well just use document.title = .

The ingest team is happy for us to drop the true flag in this PR so that monitoring also uses the baseTitle.

@rudolf ok great. then I guess I will change the type to export type ChromeDocTitleChange = string | string[]

But yes, the dual logic between displaying the title and the join(' - ') is quite annoying :)

pgayvallet · 2019-10-14T15:59:36Z

src/legacy/ui/public/doc_title/__tests__/doc_title.js

This is what I'm referring to in previous comment.

pgayvallet · 2019-10-14T16:00:31Z

src/legacy/ui/public/doc_title/doc_title.js

render seems to be an unused export, so removing it was avoiding another method bridging.

elasticmachine · 2019-10-14T17:25:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 122b7d5fae56d9533c5d291b79740870e754395e

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

…odule in tests with no usages Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

elasticmachine · 2019-10-14T20:11:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 08f20ad

joshdover

Can you go ahead and a entry in the migration guide table as part of this PR?

joshdover · 2019-10-17T19:57:57Z

src/core/public/chrome/doc_title/doc_title_service.ts

+   * @param newTitle The new title to set, either a string, string array or {@link ChromeDocTitleEntry}
+   * @param [apply=true] If false, will not actually apply the new title until #apply() is called.
+   */
+  change: (newTitle: ChromeDocTitleChange, apply?: boolean) => void;


So one annoying thing about our doc tooling is it will only generate documentation for @param tags if the function is defined as a method rather than a function property:

change(newTitle: ChromeDocTitleChange, apply?: boolean): void;

May want to do that here so that the @link is actually in the docs. Same for the other methods below.

Well, thats pretty good to know.

joshdover · 2019-10-17T20:01:34Z

src/core/public/chrome/doc_title/doc_title_service.ts

+export class DocTitleService {
+  private document = { title: '' };
+  private baseTitle = '';
+  private current = defaultTitle;


Why use an additional property instead of this.title$.value? Seems like unnecessary state to track.

indeed it is, thanks

joshdover · 2019-10-17T20:07:50Z

src/core/public/chrome/doc_title/doc_title_service.ts

+    this.title$.next(this.baseTitle);
+
+    return {
+      get$: () => this.title$.pipe(takeUntil(this.stop$)),


What actually uses this?

No one atm. Just thought it might be useful to expose it so its already present if a plugin may want to use it in the futur. Do you think it's better to just remove it ?

Yeah let's just remove it. There's some APIs similar to this in the ChromeService that I think we can also remove soon too. I think the smaller the surface the better so that when we declare the API stable we don't have to maintain anything unnecessary.

rudolf

Every small reduction / simplification in the API is a win, so I hope we can remove some of the options if they are indeed unnecessary.

src/core/public/chrome/doc_title/doc_title_service.ts

rudolf · 2019-10-18T09:30:51Z

src/core/public/chrome/doc_title/doc_title_service.ts

There's currently only one plugin that uses excludeBase=true, although we want to keep BWC in most cases in my opinion we should remove this flag and make it consistent across Kibana. I've asked the ingest team for feedback if they rely on this behaviour.

kibana/x-pack/legacy/plugins/monitoring/public/services/title.js

Lines 18 to 22 in 7ac8e4d

docTitle.change(

i18n.translate('xpack.monitoring.stackMonitoringDocTitle', {

defaultMessage: 'Stack Monitoring {clusterName} {suffix}',

values: { clusterName, suffix }

}), true);

rudolf · 2019-10-18T09:56:13Z

src/core/public/chrome/doc_title/doc_title_service.ts

I think this is a good way to bridge the old tests.

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

…title

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

…title

pgayvallet · 2019-10-23T14:16:22Z

I've made the changes.

__legacy.setBaseTitle has been kept, as without it I had to basically remove every old karma tests
I removed the apply option and logic. It was only used for the angular router events, and was not, in fact, required. Now, we only call reset during the route change start event, and the behaviour appears to be exactly the same (and CI seems to confirm this)
removed the $get and currentTitle as they were unused.
removed the excludeBase option
doc updated and entry in migration guide added.

This greatly simplify the code and test

@rudolf and @joshdover PTAL

elasticmachine · 2019-10-23T14:20:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1428149

joshdover · 2019-10-23T15:07:42Z

ack: Will re-review today

rudolf · 2019-10-24T13:05:25Z

src/core/public/chrome/doc_title/doc_title_service.ts

 * @public
 */
-export type ChromeDocTitleChange = string | string[] | ChromeDocTitleEntry;
+export type ChromeDocTitleChange = string | string[];


optional: Is there any advantage to having a named type? It feels like it might be simpler if to inline if we're only using primitive types, but happy for you to merge this either way.

I had the same question. I'm quite used to always use named types from were I worked before, but for that kind of scenario, I cant really think of any real advantage (except maybe refactoring if one day we modify/extend the type), and I agree that I doesn't really makes documentation more clear. If we think this is better I definitely can fall back to inline types

…title

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

elasticmachine · 2019-10-24T15:31:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: e4b236a

* create NP docTitle service and bridge LP to it Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * properly prefix the docTitle public types Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * update documentation Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * replace direct NP access with closure to avoid error when importing module in tests with no usages Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * remove arrow functions for doc generation Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * remove get$ from the api Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * remove apply param and excludeBase option Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * remove removed export Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * adapt legacy service to new api Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * add entry about docTitle in the migration guide Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * add link in migration guide Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * update generated doc Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * update chrome mock Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * update snapshots due to api change Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * remove ChromeDocTitleChange in favor of inline type Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

pgayvallet added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Oct 14, 2019

pgayvallet commented Oct 14, 2019

View reviewed changes

pgayvallet added 4 commits October 14, 2019 20:48

create NP docTitle service and bridge LP to it

24ecaec

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

properly prefix the docTitle public types

b0aa713

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

update documentation

261a200

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

replace direct NP access with closure to avoid error when importing m…

08f20ad

…odule in tests with no usages Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

pgayvallet force-pushed the kbn-46077-np-doc-title branch from 122b7d5 to 08f20ad Compare October 14, 2019 18:48

pgayvallet marked this pull request as ready for review October 15, 2019 05:35

pgayvallet requested a review from a team as a code owner October 15, 2019 05:35

pgayvallet self-assigned this Oct 15, 2019

pgayvallet added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.6.0 and removed release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Oct 15, 2019

joshdover reviewed Oct 17, 2019

View reviewed changes

rudolf reviewed Oct 18, 2019

View reviewed changes

pgayvallet added 4 commits October 21, 2019 08:31

remove arrow functions for doc generation

678dd50

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

remove get$ from the api

bd9d436

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

remove apply param and excludeBase option

568e923

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

remove removed export

8832e3e

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

pgayvallet added 8 commits October 23, 2019 05:31

adapt legacy service to new api

fb5b816

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

add entry about docTitle in the migration guide

12df886

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

Merge remote-tracking branch 'upstream/master' into kbn-46077-np-doc-…

37d01c8

…title

add link in migration guide

84fce61

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

update generated doc

5d2b431

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

update chrome mock

ecaa5f6

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

update snapshots due to api change

54d9567

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

Merge remote-tracking branch 'upstream/master' into kbn-46077-np-doc-…

1428149

…title

rudolf approved these changes Oct 24, 2019

View reviewed changes

pgayvallet added 2 commits October 24, 2019 07:07

Merge remote-tracking branch 'upstream/master' into kbn-46077-np-doc-…

e8edc92

…title

remove ChromeDocTitleChange in favor of inline type

e4b236a

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

pgayvallet merged commit ccd01a3 into elastic:master Oct 24, 2019

pgayvallet mentioned this pull request Oct 24, 2019

[7.x] Migrate ui/doc_title to New platform (#48121) #49264

Closed

pgayvallet mentioned this pull request Oct 24, 2019

[7.x] Migrate ui/doc_title to New platform (#48121) #49265

Merged

	docTitle.change(
	i18n.translate('xpack.monitoring.stackMonitoringDocTitle', {
	defaultMessage: 'Stack Monitoring {clusterName} {suffix}',
	values: { clusterName, suffix }
	}), true);

Conversation

pgayvallet commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Dev Docs

Uh oh!

elasticmachine commented Oct 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 14, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Oct 14, 2019

💚 Build Succeeded

Uh oh!

joshdover left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet commented Oct 23, 2019

Uh oh!

elasticmachine commented Oct 23, 2019

💚 Build Succeeded

Uh oh!

joshdover commented Oct 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

pgayvallet commented Oct 14, 2019 •

edited

Loading

pgayvallet Oct 14, 2019 •

edited

Loading

pgayvallet Oct 24, 2019 •

edited

Loading