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

[Maps] implemention of usng support fixing #76144 #107835

Merged

Conversation

mkellogg91
Copy link
Contributor

@mkellogg91 mkellogg91 commented Aug 6, 2021

Summary

Adds usng.js library for Lat Lon, UTM, and MGRS conversion instead of using MGRS and UTM libraries. Fix and images of UI shown in #76144.

Release note

Adds support for UTM and MGRS coordinates in Maps "Go To" button.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@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?

@mkellogg91
Copy link
Contributor Author

mkellogg91 commented Aug 6, 2021

@nickpeihl @nreese didn't add the "review" label to the pr before it was created and am unsure how to add it now.

@nickpeihl nickpeihl added the [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation label Aug 6, 2021
@elasticmachine
Copy link
Contributor

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

@nickpeihl
Copy link
Member

nickpeihl commented Aug 6, 2021

Hi @mkellogg91. Thanks for the PR. We'll handle the labels.

@elasticmachine test this please

@mkellogg91
Copy link
Contributor Author

@nickpeihl anything further required from me, or just waiting on approval?

@thomasneirynck thomasneirynck changed the title implemention of usng support fixing #76144 [Maps] implemention of usng support fixing #76144 Aug 9, 2021
Copy link
Member

@nickpeihl nickpeihl 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 @mkellogg91.

In addition to my inline comments, there are some linting and typing issues that need to be resolved for our tests to pass.

  1. Lint errors. Probably many of these can be fixed automatically by running node scripts/eslint "x-pack/**/set_view_control.tsx" --fix.
  2. Type checks need to be resolved. You can also run type checking locally by running node scripts/type_check.js --project="x-pack/plugins/maps/tsconfig.json"
  3. We need to pull master into your branch.
> git pull upstream master
> git push

@mkellogg91 mkellogg91 requested a review from a team as a code owner August 13, 2021 18:07
@cla-checker-service
Copy link

cla-checker-service bot commented Aug 13, 2021

💚 CLA has been signed

@nickpeihl
Copy link
Member

nickpeihl commented Aug 16, 2021

Hi @mkellogg91. Can you please sign the Contributor Agreement linked above? We can not merge this PR without it. Thanks.

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

this is great.

@mkellogg91 if you sign the CLA, Kibana's CI will run the test suite.

I also put some initial version labels on this, a requirement for CI to be green.


Just a few nits from my end. See comments

Also, just so i understand, but should changing the UTM-zone, also not shift the location of the center?

Aug-18-2021 16-59-59

@mkellogg91
Copy link
Contributor Author

Hi @mkellogg91. Can you please sign the Contributor Agreement linked above? We can not merge this PR without it. Thanks.

Signed. Was out for a week, but have returned now and will work on getting all the requested changes above completed

@mkellogg91
Copy link
Contributor Author

@nickpeihl I've submitted changes according to your requests. Take a look and let me know what you think

@mkellogg91
Copy link
Contributor Author

this is great.

@mkellogg91 if you sign the CLA, Kibana's CI will run the test suite.

I also put some initial version labels on this, a requirement for CI to be green.

Just a few nits from my end. See comments

Also, just so i understand, but should changing the UTM-zone, also not shift the location of the center?

Aug-18-2021 16-59-59

this was a good catch. It wasn't working properly before. you should find that it works now

Copy link
Member

@nickpeihl nickpeihl 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 fixes @mkellogg91. I added some suggestions I think we can use to avoid using the any type. But please verify that my suggestions are accurate.

@nickpeihl
Copy link
Member

Hi @mkellogg91. This is looking good.

  1. Would you mind enabling allow edits from maintainers on this PR? It will help us keep the master branch in sync and keep CI running.

  2. Can you also take a look at this suggestion by @thomasneirynck?

Once we address those two items, I think we can merge.

@mkellogg91 mkellogg91 marked this pull request as draft September 8, 2021 18:00
@mkellogg91 mkellogg91 marked this pull request as ready for review September 8, 2021 18:01
@mkellogg91
Copy link
Contributor Author

Hi @mkellogg91. This is looking good.

1. Would you mind enabling [allow edits from maintainers](https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests) on this PR? It will help us keep the master branch in sync and keep CI running.

2. Can you also take a look at [this suggestion by @thomasneirynck](https://github.com/elastic/kibana/pull/107835#discussion_r691596953)?

Once we address those two items, I think we can merge.

@nickpeihl I do not see this option in the pull request...is it possible I don't have permission to do this?

@nickpeihl
Copy link
Member

@nickpeihl I do not see this option in the pull request...is it possible I don't have permission to do this?

Weird. On PRs I create there is an option on the right sidebar that looks like this. If it's there, please make sure the box is checked.

Screen Shot 2021-09-08 at 11 08 57 AM

If it isn't there for you, I wonder if you have a permissions issue with the branch in the spetriclabs fork?

@mkellogg91
Copy link
Contributor Author

Hi @mkellogg91. Thanks for the PR. We'll handle the labels.

@elasticmachine test this please

image

@nickpeihl
Copy link
Member

image

Hmm, in that case, you will need to keep the PR branch in sync with master since we can't do that. If you have elastic/kibana as the upstream remote, then you should just be able to:

> git pull upstream master
> git push

So please address this suggestion and pull the changes from the master branch. Then I'll trigger the CI to run and hopefully we can merge.

@mkellogg91
Copy link
Contributor Author

image

Hmm, in that case, you will need to keep the PR branch in sync with master since we can't do that. If you have elastic/kibana as the upstream remote, then you should just be able to:

> git pull upstream master
> git push

So please address this suggestion and pull the changes from the master branch. Then I'll trigger the CI to run and hopefully we can merge.

Will do..I'll work on addressing that suggestion.

@mkellogg91
Copy link
Contributor Author

mkellogg91 commented Sep 8, 2021

@nickpeihl just pushed changes for unused code paths and pulled in upstream changes from master

@nickpeihl
Copy link
Member

Jenkins, test this

@nickpeihl
Copy link
Member

@mkellogg91 looks like we're seeing some lint errors. Can you run node scripts/eslint "x-pack/**/set_view_control.tsx" --fix again to see if that fixes them?

@mkellogg91
Copy link
Contributor Author

@mkellogg91 looks like we're seeing some lint errors. Can you run node scripts/eslint "x-pack/**/set_view_control.tsx" --fix again to see if that fixes them?

Fixed linting issues and pulled upstream master, then pushed changes again to feature branch.

@nickpeihl
Copy link
Member

jenkins, test this

@mkellogg91
Copy link
Contributor Author

jenkins, test this

after this PR makes it in I'm going to put "Official ElasticSearch and Kibana Git Contributer" on my linkedin and be showered with cash

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
maps 846 847 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
maps 3.2MB 3.2MB +30.7KB

History

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

@nickpeihl nickpeihl self-requested a review September 9, 2021 21:26
@nickpeihl nickpeihl self-requested a review September 9, 2021 21:33
Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

lgtm!

code review and tested in firefox.

Thanks @mkellogg91 for your contribution to Kibana!

@nickpeihl nickpeihl added the auto-backport Deprecated: Automatically backport this PR after it's merged label Sep 9, 2021
@nickpeihl nickpeihl merged commit 5acd132 into elastic:master Sep 9, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 9, 2021
)

* implemention of usng support fixing elastic#76144

* linting fixes

* lint fixes

* pr code edits

* fixed utm zone and added error proofing

* removing any data types and replacing with explicit ones

* avoiding render of unused components

* fixing linting issues

Co-authored-by: Michael Ihde <mihde@spectric.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 10, 2021
* implemention of usng support fixing #76144

* linting fixes

* lint fixes

* pr code edits

* fixed utm zone and added error proofing

* removing any data types and replacing with explicit ones

* avoiding render of unused components

* fixing linting issues

Co-authored-by: Michael Ihde <mihde@spectric.com>

Co-authored-by: Michael Kellogg <mkellogg91@gmail.com>
Co-authored-by: Michael Ihde <mihde@spectric.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged 💝community [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:enhancement v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants