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

Convert map client routing files to TS #76387

Merged
merged 13 commits into from
Sep 8, 2020

Conversation

shamin
Copy link
Contributor

@shamin shamin commented Sep 1, 2020

Summary

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@shamin shamin requested a review from a team as a code owner September 1, 2020 14:57
@kibanamachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@nreese
Copy link
Contributor

nreese commented Sep 1, 2020

retest

@nreese nreese added release_note:skip Skip the PR/issue when compiling release notes [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.10.0 v8.0.0 labels Sep 1, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@shamin shamin requested a review from a team as a code owner September 1, 2020 18:18
@shamin
Copy link
Contributor Author

shamin commented Sep 1, 2020

I have fixed the type check errors and eslint errors shown by the Jenkins.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Thank you for submitting this PR. It great to see these files converted to TS.

@shamin shamin force-pushed the maps/routing-typescript branch from 8680384 to 765f772 Compare September 2, 2020 16:16
@shamin
Copy link
Contributor Author

shamin commented Sep 2, 2020

@nreese I have made the changes you suggested. Also fixed some merge conflicts.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Thanks for the clean-up. This is looking really great. Just some more suggestions to get read of any and some other typings clean-up

@shamin
Copy link
Contributor Author

shamin commented Sep 3, 2020

@nreese Made the changes you suggested.

@nreese
Copy link
Contributor

nreese commented Sep 3, 2020

retest

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Thanks for all of the changes. Renaming savedQuery to savedQueryId in AppState really helps clarify things.

Mind syncing with master to fix merge conflicts?

git fetch upstream
git pull upstream master
git push origin {branch_name}

@nreese
Copy link
Contributor

nreese commented Sep 8, 2020

@elasticmachine merge upstream

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Code owner change is TS export LGTM

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Thanks for all of the updates. Just a few more comments

@shamin
Copy link
Contributor Author

shamin commented Sep 8, 2020

@nreese Made the changes you suggested.

@nreese
Copy link
Contributor

nreese commented Sep 8, 2020

retest

@nreese
Copy link
Contributor

nreese commented Sep 8, 2020

@shamin Looks like CI is failing because some autogenerated API documentation needs to be updated.

See below commands from https://github.com/elastic/kibana/pull/76387/checks?check_run_id=1086876819 to update the documentation and commit the changes.

Screen Shot 2020-09-08 at 10 14 42 AM

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Just a few more details

@shamin
Copy link
Contributor Author

shamin commented Sep 8, 2020

@nreese Made the changes you suggested. Also updated the autogenerated docs.

@nreese
Copy link
Contributor

nreese commented Sep 8, 2020

retest

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
maps 3.3MB +451.0B 3.3MB

page load bundle size

id value diff baseline
data 1.4MB +206.0B 1.4MB

History

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

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Thanks for contributing to Kibana. These are great changes that are really bringing the typings together.

LGTM
code review

@nreese nreese merged commit 194d0b0 into elastic:master Sep 8, 2020
nreese pushed a commit to nreese/kibana that referenced this pull request Sep 8, 2020
* Converted maps_router and store operations files

* Converted files in map routes to typescript

* Removed an unwanted ts-expect-error

* Fixed the lint errors from jenkins

* Naming changes, type for mapStateJSON etc.

* More type fixes in map routes

* More type fixes in map routes

* Added back some removed props

* Added types to app state manager

* Autogenerated api documentation

* Type fixes

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@nreese
Copy link
Contributor

nreese commented Sep 8, 2020

backport 7.x #76991

nreese added a commit that referenced this pull request Sep 9, 2020
* Converted maps_router and store operations files

* Converted files in map routes to typescript

* Removed an unwanted ts-expect-error

* Fixed the lint errors from jenkins

* Naming changes, type for mapStateJSON etc.

* More type fixes in map routes

* More type fixes in map routes

* Added back some removed props

* Added types to app state manager

* Autogenerated api documentation

* Type fixes

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Shamin Meerankutty <8272719+shamin@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💝community [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] Convert client routing files to TS
5 participants