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

Start using useKibana context in data shim #46478

Merged

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Sep 24, 2019

Summary

Turn React class components into functional components (to make using context easier) and use KibanaContext for core services

  • FilterBar
  • QueryBarInputRow

Checklist

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

For maintainers

Liza K added 3 commits September 24, 2019 16:14
Pass deps in using Context provider
Use notification.toasts in SearchBar
Use context provider in top nav.
@lizozom lizozom changed the title Newplatform/data plugin/stateful search bar Start using useKibana context in data shim Sep 24, 2019
@lizozom lizozom self-assigned this Sep 24, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@lizozom lizozom added the release_note:skip Skip the PR/issue when compiling release notes label Sep 25, 2019
@streamich streamich self-assigned this Sep 26, 2019
Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

LGTM, checked on Mac/Chrome, added few minor comments below.

function handleLuceneSyntaxWarning() {
if (!props.query) return;
const { intl, store } = props;
const { query, language } = props.query;
Copy link
Contributor

Choose a reason for hiding this comment

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

This handleLuceneSyntaxWarning is quite large and does not deal with rendering per-se, but displays an error message, if one happens. Maybe this function could be lifted out of the rendering path, now every time QueryBarTopRowUI is rendered it creates a new handleLuceneSyntaxWarning function. This function really depends only on store and query, they could be passed in:

showLuceneSyntaxWarning(props.store, props.query);

BTW, to use internationalisation, we don't need to use injectI18n HOC, I believe it can be just statically imported import {i18n} from '@kbn/i18n'.

}
/>

<KibanaContextProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we could use connected <SearchBar> component, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the next PR!

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Changes in Lens LGTM. QueryBarInput is used in x-pack/legacy/plugins/lens/public/indexpattern_plugin/operations/definitions/filter_ratio.tsx, using the context there would simplify dependency passing in Lens, but it's probably already on the list :)

@lizozom lizozom merged commit 09ea3d5 into elastic:master Sep 26, 2019
@streamich streamich added this to In progress in kibana-app-arch via automation Sep 30, 2019
@streamich streamich moved this from In progress to Done in 7.5 in kibana-app-arch Sep 30, 2019
@lizozom lizozom deleted the newplatform/data-plugin/stateful-search-bar branch November 14, 2019 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants