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

Use platform history #74328

Merged
merged 35 commits into from
Aug 31, 2020
Merged

Use platform history #74328

merged 35 commits into from
Aug 31, 2020

Conversation

smith
Copy link
Contributor

@smith smith commented Aug 5, 2020 •
edited
Loading

  • Replace the hash history with the platform history in the Observability and APM plugins
  • Replace the global history object with React Router's useHistory
  • Add the RedirectAppLinks component
  • Use navigateToUrl in breadcrumbs so they work with the new router
  • Redirect old hash-style URLs to new-style URLs
  • Update the signature of getAPMHref to look more like getInfraHref and take a basePath
  • Remove navigateToApp calls on the actions menu since they are no longer needed

Also:

  • Use CoreStart instead of AppMountContext['core'] (this was left over from earlier migration to KP and is more correct)
  • Reorder some imports
  • Add some tests
  • Add some missing contexts to get rid of warnings about usageCollection in tests
  • Mock the console in some tests to get rid of warnings caused by external dependencies
  • Replace AppMountContextBasePath with IBasePath since that's what core.http.baseBath is

Fixes #72764. Fixes #65682. Fixes #69817.

This does not attempt to address #51963 at all, though that could be done in a follow-up PR.

@smith smith marked this pull request as ready for review August 7, 2020 00:37
@smith smith requested a review from a team as a code owner August 7, 2020 00:37
@smith smith changed the title Use platform router Use platform history Aug 7, 2020
@cauemarcondes
Copy link
Contributor

@elasticmachine merge upstream

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Aug 11, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@cauemarcondes
Copy link
Contributor

@dgieselaar the issue has been merged, can you point where you see the unnecessary changes?

shahzad31
shahzad31 approved these changes Aug 24, 2020
View reviewed changes
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 !!

@smith smith requested a review from dgieselaar August 25, 2020 15:20
sorenlouv
sorenlouv reviewed Aug 26, 2020
View reviewed changes
x-pack/plugins/apm/public/utils/history.ts
history = createHashHistory();
};

export { history };
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@smith
Copy link
Contributor Author

smith commented Aug 26, 2020

@elasticmachine merge upstream

sorenlouv
sorenlouv reviewed Aug 26, 2020
View reviewed changes
...k/plugins/apm/public/components/app/Main/route_config/route_handlers/agent_configuration.tsx
@@ -40,6 +41,7 @@ export function EditAgentConfigurationRouteHandler() {
}

export function CreateAgentConfigurationRouteHandler() {
const history = useHistory();
Copy link
Member

Choose a reason for hiding this comment

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

Is this the same history object as the platform history? If not, is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. useHistory uses the history provided to the router, which we do here: https://github.com/elastic/kibana/pull/74328/files#diff-0d2b10d4756fabbe9a3043f569b0b9d3R93.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect!

@smith
Copy link
Contributor Author

smith commented Aug 27, 2020

@elasticmachine merge upstream

@dgieselaar dgieselaar removed their request for review August 28, 2020 09:52
@dgieselaar dgieselaar dismissed their stale review August 28, 2020 09:54

Looks like the requested changes were made. I've not looked at the other code.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
apm 938 -7 945
observability 89 -6 95
total -13

async chunks size

id value diff baseline
apm 4.7MB -12.8KB 4.7MB
observability 296.9KB -13.4KB 310.3KB
total -26.3KB

History

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

@smith smith merged commit 9b59122 into elastic:master Aug 31, 2020
@kibanamachine
Copy link
Contributor

💔 Backport was not successful

The PR was attempted backported to the following branches:

  • ❌ 7.x: An unhandled error occurred. Please consult the logs

@smith smith deleted the nls/new-history branch August 31, 2020 13:43
@smith smith mentioned this pull request Aug 31, 2020
Merged
smith added a commit to smith/kibana that referenced this pull request Aug 31, 2020
@smith @elasticmachine @cauemarcondes
Use platform history (elastic#74328)
d5fd235 
Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: cauemarcondes <[email protected]>
smith added a commit that referenced this pull request Aug 31, 2020
@smith @cauemarcondes
Use platform history (#74328) (#76287)
ac29d33 
Co-authored-by: cauemarcondes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cauemarcondes cauemarcondes cauemarcondes approved these changes

@shahzad31 shahzad31 shahzad31 approved these changes

@sorenlouv sorenlouv sorenlouv approved these changes

@dgieselaar dgieselaar dgieselaar left review comments

Assignees
No one assigned
Labels
release_note:enhancement Team:APM All issues that need APM UI Team support v7.10.0
Projects
None yet
Milestone
No milestone
7 participants
@smith @cauemarcondes @elasticmachine @kibanamachine @sorenlouv @dgieselaar @shahzad31