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

KibanaContext in index pattern managment ui #66985

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented May 19, 2020

Summary

Simplified component dependencies with KibanaContext

@VladLasitsa VladLasitsa requested a review from alexwizp May 19, 2020 12:24
@VladLasitsa VladLasitsa self-assigned this May 19, 2020
@VladLasitsa VladLasitsa added Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages release_note:skip Skip the PR/issue when compiling release notes Team:AppArch v7.9.0 v8.0.0 labels May 19, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@VladLasitsa VladLasitsa marked this pull request as ready for review May 19, 2020 14:31
@VladLasitsa VladLasitsa requested a review from a team as a code owner May 19, 2020 14:31
Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

It's a great improvement! Thank you. @mattkime could you please review

@VladLasitsa VladLasitsa force-pushed the kibana_context_in_index_pattern_managment_ui branch from 726d5e3 to 280aa24 Compare May 20, 2020 12:25
@cchaos cchaos removed the request for review from a team May 20, 2020 14:42
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.

Code changes LGTM.

@@ -66,6 +60,10 @@ interface StepIndexPatternState {
}

export class StepIndexPattern extends Component<StepIndexPatternProps, StepIndexPatternState> {
static contextType = contextType;

declare readonly context: IndexPatternManagmentContextValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, maybe it is more future-proof to use withKibana higher order component instead of this legacy context API.

export const StepIndexPattern = withKibana(StepIndexPatternPure);

Copy link
Contributor

@streamich streamich May 20, 2020

Choose a reason for hiding this comment

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

Or useKibana hook

export const StepIndexPattern = props => {
  const {services} = useKibana();
  return <StepIndexPatternPure {...props} services={services} />;
};

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 it's just a different way to pass the context. I found out that we use both ways in the application. Regarding use Kibana hook, I used it for functional components.

@mattkime
Copy link
Contributor

overall the changes look good but this could use another pass to use useKibana and withKibana from the kibana-react plugin.

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@VladLasitsa VladLasitsa merged commit 0ab55da into elastic:master May 25, 2020
VladLasitsa added a commit to VladLasitsa/kibana that referenced this pull request May 25, 2020
* Using KibanaContext instead of passing dependencies.

* Fixed comments

* Delete index.scss

* Added comment for workaround

* Fixed tests

* Fixed eslint

Co-authored-by: Alexey Antonov <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
VladLasitsa added a commit that referenced this pull request May 25, 2020
* Using KibanaContext instead of passing dependencies.

* Fixed comments

* Delete index.scss

* Added comment for workaround

* Fixed tests

* Fixed eslint

Co-authored-by: Alexey Antonov <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Alexey Antonov <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
jloleysens added a commit that referenced this pull request May 26, 2020
…ine-editor

* 'master' of github.com:elastic/kibana: (129 commits)
  [Canvas] Force embeddables to refresh when renderable reevaluated (#67133)
  [Canvas] Better handling navigating to/from canvas (#66407)
  [Ingest pipelines] Fix schema validation for simulate and update routes (#67199)
  do not use es from setup (#67277)
  Auto expand replicas for event log (#67286)
  Observability & APM do not use elasticsearch client provided via setup contract  (#67263)
  Fix privileges check when security is not enabled (#67308)
  add IIS home (#66918)
  [ML] Adding additional job service endpoint tests (#66892)
  [Ingest Manager] Update fleet internal doc with latest flags (#67193)
  [Discover] Deangularize the loading spinner (#67165)
  Add `application.navigateToUrl` core API (#67110)
  Improve indexpattern without timefield functional test (#67031)
  KibanaContext in index pattern managment ui (#66985)
  Fix Azure metrics tutorial inside the App Home/ Add data area (#66901)
  add azure logs home (#66910)
  fix: rum agent should work correctly on new platform (#67037)
  [test_utils/Testbed] Move to src/test_utils folder (OSS) (#66898)
  only block registration when appRoute contains the exact basePath (#67125)
  Changed actions API endpoints urls to follow Kibana STYLEGUIDE (#65936)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants