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

[NP] Remove absoluteToParsedUrl & KibanaParsedUrl ref in kibana app #61105

Merged
merged 11 commits into from
Mar 30, 2020
Merged

[NP] Remove absoluteToParsedUrl & KibanaParsedUrl ref in kibana app #61105

merged 11 commits into from
Mar 30, 2020

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Mar 24, 2020 •
edited
Loading

Summary

This removes absoluteToParsedUrl & KibanaParsedUrl from dashboard/visualize and uses simple url helpers instead.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@sulemanof sulemanof changed the title Shim/remove kibana parsed url [NP] Rremove absoluteToParsedUrl & KibanaParsedUrl ref in kibana app Mar 24, 2020
This was referenced Mar 24, 2020
Closed
Closed
@sulemanof
Merge remote-tracking branch 'upstream/master' into shim/remove_kiban…
9d1a9a9 
…a_parsed_url

# Conflicts:
#	x-pack/legacy/plugins/lens/public/plugin.tsx
@sulemanof
Fix saved dashboard
64bea6e
@sulemanof sulemanof added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0 labels Mar 25, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@sulemanof sulemanof marked this pull request as ready for review March 25, 2020 10:53
@sulemanof sulemanof requested a review from a team March 25, 2020 10:53
@sulemanof
Copy link
Contributor Author

My only concern here is usage of DashboardConstants in visualise and lens plugins!
@flash1293 is there a plan to make it more common? or is it a right way to stay it there?

@flash1293
Copy link
Contributor

Importing a constant from another plugin shouldn't be a problem when it's exposed from the top level, but maybe I'm missing something. @sulemanof what's your concern?

@kertal kertal changed the title [NP] Rremove absoluteToParsedUrl & KibanaParsedUrl ref in kibana app [NP] Remove absoluteToParsedUrl & KibanaParsedUrl ref in kibana app Mar 25, 2020
@flash1293 flash1293 added v7.8.0 and removed v7.7.0 labels Mar 26, 2020
flash1293
flash1293 approved these changes Mar 26, 2020
View reviewed changes
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.

Tested and adding visualizations / lenses from dashboard still works fine, LGTM.

As discussed offline the current setup for URL building is a temporary state and should be unified so dashboards own building dashboard URLs with a fixed set of parameters

@sulemanof
Merge remote-tracking branch 'upstream/master' into shim/remove_kiban…
b994157 
…a_parsed_url

# Conflicts:
#	x-pack/legacy/plugins/lens/public/plugin.tsx
@sulemanof
Merge remote-tracking branch 'upstream/master' into shim/remove_kiban…
d709340 
…a_parsed_url

# Conflicts:
#	x-pack/legacy/plugins/lens/public/legacy_imports.ts
@sulemanof
Fix empty line after resolving conflicts
922c9c1
@kertal
Copy link
Member

kertal commented Mar 30, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@sulemanof sulemanof merged commit 84d1bbd into elastic:master Mar 30, 2020
@sulemanof sulemanof deleted the shim/remove_kibana_parsed_url branch March 30, 2020 08:51
@sulemanof sulemanof mentioned this pull request Mar 30, 2020
Merged
sulemanof added a commit to sulemanof/kibana that referenced this pull request Mar 30, 2020
@sulemanof @elasticmachine
[NP] Remove absoluteToParsedUrl & KibanaParsedUrl ref in kibana app (e…
9bbda98 
…lastic#61105)

* Remove absoluteToParsedUrl reference in dashboard

* Remove KibanaParsedUrl from visualize

* Fix tests

* Add tests

* Fix saved dashboard

* Fix empty line after resolving conflicts

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
sulemanof added a commit that referenced this pull request Mar 30, 2020
@sulemanof @elasticmachine
[NP] Remove absoluteToParsedUrl & KibanaParsedUrl ref in kibana app (#…
e146ae0 
…61105) (#61762)

* Remove absoluteToParsedUrl reference in dashboard

* Remove KibanaParsedUrl from visualize

* Fix tests

* Add tests

* Fix saved dashboard

* Fix empty line after resolving conflicts

Co-authored-by: Elastic Machine <elasticmachine@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
Reviewers

@flash1293 flash1293 flash1293 approved these changes

@majagrubic majagrubic Awaiting requested review from majagrubic

@kertal kertal Awaiting requested review from kertal

Assignees
No one assigned
Labels
backported Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sulemanof @elasticmachine @flash1293 @kertal @kibanamachine