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.

Sign up for GitHub

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

Jump to bottom

Improve Migration guide with examples and links #46661

Merged
merged 2 commits into from
Oct 16, 2019
Merged

Improve Migration guide with examples and links #46661

merged 2 commits into from
Oct 16, 2019

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Sep 25, 2019 •
edited
Loading

Summary

Depends on #47069

See last commit for changes in this PR.

This includes:

  • Updated translation tables for LP -> NP core services
  • New translation table for uiExports
  • More fleshed out examples in MIGRATION_EXAMPLES.md
  • Many, many more links to NP API documentation

[skip-ci]

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover joshdover force-pushed the np/migration-guide-updates branch 2 times, most recently from 7d28da2 to 0463e13 Compare October 8, 2019 19:36
@joshdover joshdover added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 labels Oct 8, 2019
@elasticmachine
Copy link
Contributor

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

@joshdover joshdover marked this pull request as ready for review October 8, 2019 19:38
@joshdover joshdover requested review from a team as code owners October 8, 2019 19:38
eliperelman
eliperelman previously requested changes Oct 10, 2019
View reviewed changes
Copy link
Contributor

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

This looks fantastic, just have a few minor things

src/core/MIGRATION.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/public/notifications/toasts/toasts_api.tsx Outdated
public registerOverlays(overlays: OverlayStart) {
this.overlays = overlays;
}

/** Current array of toast messages to show to user. */
Copy link
Contributor

Choose a reason for hiding this comment

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

This returns an observable, should the message here change to not say array?

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 will address this in the original PR: #47069

mshustov
mshustov reviewed Oct 14, 2019
View reviewed changes
src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md Outdated
defaultAppId: schema.string({ defaultValue: true }),
index: schema.string({ defaultValue: '.kibana' }),
disableWelcomeScreen: schema.boolean({ defaultValue: false }),
autocompleteTerminateAfter: schema.number({ min: 1, defaultValue: 100000 }),
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: sometimes schema.duration from @kbn/config-schema is more appropriate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't currently support min

src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md Outdated Show resolved Hide resolved
src/core/MIGRATION_EXAMPLES.md
}
```

## Chrome
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we separate examples by server-path / client-path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean. I don't believe we have any chrome APIs on the server? Or are you talking about basePath APIs?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird to jump to Chrome without referencing to the client side.
I'd even split the manual into 2 different documents to be more focused.

src/core/public/notifications/toasts/toasts_service.mock.ts Outdated Show resolved Hide resolved
@joshdover joshdover requested a review from a team as a code owner October 14, 2019 21:30
sorenlouv
sorenlouv approved these changes Oct 15, 2019
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

apm changes lgtm

@joshdover joshdover mentioned this pull request Oct 15, 2019
Closed
8 tasks
@joshdover
Copy link
Contributor Author

Merging now, if there any further ideas on improvements, please post in #48245

@joshdover joshdover merged commit f82c2e9 into elastic:master Oct 16, 2019
@joshdover joshdover deleted the np/migration-guide-updates branch October 16, 2019 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mw-ding mw-ding mw-ding left review comments

@sorenlouv sorenlouv sorenlouv approved these changes

@eliperelman eliperelman eliperelman approved these changes

@mshustov mshustov Awaiting requested review from mshustov

Assignees
No one assigned
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joshdover @elasticmachine @sorenlouv @eliperelman @mw-ding @mshustov