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

Converted brush event to TS. Migrated tests to "jest" way and optimiz… #58206

Merged
merged 4 commits into from
Feb 26, 2020
Merged

Converted brush event to TS. Migrated tests to "jest" way and optimiz… #58206

merged 4 commits into from
Feb 26, 2020

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Feb 21, 2020 •
edited by alexwizp
Loading

Closes: #57395

Summary

src/legacy/core_plugins/data/public/actions/filters/brush_event.js

  • convert file (and tests) to TS
  • convert tests to Jest
  • tests seem to be using @kbn/expect which probably isn't necessary as we could use jest assertions

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cla-checker-service
Copy link

cla-checker-service bot commented Feb 21, 2020 •
edited
Loading

💚 CLA has been signed

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp alexwizp force-pushed the _filters/brush_event]-Convert_to_TS branch from 67a6831 to 1f93355 Compare February 21, 2020 09:49
@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp alexwizp force-pushed the _filters/brush_event]-Convert_to_TS branch from 1b93c43 to b4c3bbe Compare February 21, 2020 09:57
@elastic elastic deleted a comment from kibanamachine Feb 21, 2020
@elasticmachine
Copy link
Contributor

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

@alexwizp alexwizp marked this pull request as ready for review February 21, 2020 10:02
@alexwizp alexwizp requested a review from a team as a code owner February 21, 2020 10:02
alexwizp
alexwizp approved these changes Feb 21, 2020
View reviewed changes
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.

LGTM! Thank you

@alexwizp
Copy link
Contributor

@elasticmachine merge upstream

@rayafratkina
Copy link
Contributor

Jenkins, test this

lukeelmers
lukeelmers reviewed Feb 21, 2020
View reviewed changes
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Added two comments, but overall LGTM once we get a green build. Thanks @VladLasitsa!

src/legacy/core_plugins/data/public/actions/filters/brush_event.ts Outdated Show resolved Hide resolved
src/legacy/core_plugins/data/public/actions/select_range_action.ts Outdated Show resolved Hide resolved
@lukeelmers
Copy link
Member

@elasticmachine merge upstream

@lukeelmers
Copy link
Member

Jenkins, test this

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

ppisljar
ppisljar reviewed Feb 24, 2020
View reviewed changes
src/legacy/core_plugins/data/public/actions/filters/brush_event.ts Outdated Show resolved Hide resolved
src/legacy/core_plugins/data/public/search/index.ts Outdated Show resolved Hide resolved
src/legacy/core_plugins/data/public/actions/filters/brush_event.ts
import { deserializeAggConfig } from '../../search';
// should be removed after moving into new platform plugins data folder
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { getIndexPatterns } from '../../../../../../plugins/data/public/services';
Copy link
Member

Choose a reason for hiding this comment

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

i think (i am hopefully wrong) onBrushEvent might be statically exported on the contract ? if it is we shouldn't be using state inside it (getIndexPatterns should be passed in as a paramter)

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 investigated this. onBrushEvent was imported only on one place - SelectRengeAction. Moreover, previously we imported getIndexPattern inside SelectRengeAction and just pass to onBrushEvent. Because of this, I decided to move import inside onBrushEvent. I cannot see any reason to think that onBrushEvent might be statically exported on the contract.

@alexwizp alexwizp force-pushed the _filters/brush_event]-Convert_to_TS branch from c1c2cef to c23f39f Compare February 24, 2020 13:31
@alexwizp
Copy link
Contributor

retest

@alexwizp
Copy link
Contributor

Jenkins, test this

@VladLasitsa
Copy link
Contributor Author

Jenkins, test this

1 similar comment
@rayafratkina
Copy link
Contributor

Jenkins, test this

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

Jenkins, test this

@VladLasitsa
Copy link
Contributor Author

jenkins test this please

@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

ppisljar
ppisljar approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@VladLasitsa VladLasitsa merged commit 342e501 into elastic:master Feb 26, 2020
@VladLasitsa VladLasitsa mentioned this pull request Feb 26, 2020
Merged
VladLasitsa added a commit to VladLasitsa/kibana that referenced this pull request Feb 26, 2020
@VladLasitsa @alexwizp @elasticmachine
Converted brush event to TS. Migrated tests to "jest" way and optimiz… (
73703a8 
elastic#58206)

* Converted brush event to TS. Migrated tests to "jest" way and optimiz

* Removed unused definition in interface. Revert changes related to import deserializeAggConfig.

Co-authored-by: Alexey Antonov <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
VladLasitsa added a commit that referenced this pull request Feb 26, 2020
@VladLasitsa @alexwizp @elasticmachine
Converted brush event to TS. Migrated tests to "jest" way and optimiz… (
003557f 
#58206) (#58594)

* Converted brush event to TS. Migrated tests to "jest" way and optimiz

* Removed unused definition in interface. Revert changes related to import deserializeAggConfig.

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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukeelmers lukeelmers lukeelmers left review comments

@alexwizp alexwizp alexwizp approved these changes

@ppisljar ppisljar ppisljar approved these changes

@lizozom lizozom Awaiting requested review from lizozom

Assignees

@VladLasitsa VladLasitsa

Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes review v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[filters/brush_event]: Convert to TS / Jest
7 participants
@VladLasitsa @elasticmachine @alexwizp @rayafratkina @lukeelmers @kibanamachine @ppisljar