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

[Uptime] Modify router to use ScopedHistory #76421

Merged
merged 18 commits into from
Sep 8, 2020

Conversation

justinkambic
Copy link
Contributor

Summary

Fixes #76348.

Removes the usage of BrowserRouter and reliance on hashbang routing.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@justinkambic justinkambic added v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 1, 2020
@justinkambic justinkambic requested a review from a team as a code owner September 1, 2020 18:36
@justinkambic justinkambic self-assigned this Sep 1, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for handling this that quickly.

@@ -84,7 +84,7 @@ export class UptimePlugin
});

core.application.register({
appRoute: '/app/uptime#/',
appRoute: '/app/uptime',
Copy link
Contributor

Choose a reason for hiding this comment

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

this optional btw, platform will generate default route. So we can remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I removed it c3fa61b.

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

It breaks the breadcrumb behaviour clicking uptime from details page trigger full reload.

@justinkambic
Copy link
Contributor Author

It breaks the breadcrumb behaviour clicking uptime from details page trigger full reload.

Glad you noticed this too, any idea what's causing the behavior? I'm having trouble isolating it.

@justinkambic
Copy link
Contributor Author

@shahzad31 give it another look I did something similar to APM's solution to this problem: 0e4ff0a

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@shahzad31
Copy link
Contributor

All looks good, but this last thing can you please see why details page breadcrumb is adding this flag to url
image

@justinkambic
Copy link
Contributor Author

@shahzad31 give it another look, I think all is well now.

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM !!

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
uptime 1.6MB +1.0KB 1.6MB

page load bundle size

id value diff baseline
uptime 25.1KB -59.0B 25.1KB

History

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

@justinkambic justinkambic merged commit 4afa2d6 into elastic:master Sep 8, 2020
justinkambic added a commit to justinkambic/kibana that referenced this pull request Sep 8, 2020
* Remove hashbang and modify router to use ScopedHistory.

* Update test to conform to refactored API.

* Update test snapshots.

* Fix broken type check.

* Remove unneeded prop.

* Prevent full page reload for breadcrumbs.

* Fix outdated test.

* Fix type errors.

* Add default value for focusConnectorField url param.

* Make stringify function support focusFieldConnector empty values.

* Avoid unnecessary text in breadcrumb href.

* Refresh test snapshot.

Co-authored-by: Elastic Machine <[email protected]>
justinkambic added a commit that referenced this pull request Sep 8, 2020
* Remove hashbang and modify router to use ScopedHistory.

* Update test to conform to refactored API.

* Update test snapshots.

* Fix broken type check.

* Remove unneeded prop.

* Prevent full page reload for breadcrumbs.

* Fix outdated test.

* Fix type errors.

* Add default value for focusConnectorField url param.

* Make stringify function support focusFieldConnector empty values.

* Avoid unnecessary text in breadcrumb href.

* Refresh test snapshot.

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

Co-authored-by: Elastic Machine <[email protected]>
@justinkambic
Copy link
Contributor Author

@justinkambic justinkambic deleted the uptime_hashbang-removal branch September 8, 2020 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from hash based routing
5 participants