Skip to content

Conversation

@smith
Copy link
Contributor

@smith smith commented Sep 10, 2020

These changes make the map work better with auto-refresh and with dragged nodes.

  • Extract all event handling out of the Cytoscape component into a hook.
  • Use React.memo to only render when the list of element ids has changed.
  • Only do a fit on the layout if we're going from 0 to more than 0 elements.
  • Instead of removing all the nodes on rerender, only remove the ones that aren't in the new list.
  • Trigger a custom:data event instead of data when we receive fetched data. Before we triggered a data event which would trigger a layout if you called data() on an element.
  • Don't trigger a deselect when we get new data, so popovers stay open when we get new data.
  • Animate the layout on changes.
  • When we do a layout, exclude selected nodes and nodes that have been dragged.

When we set the time range to something low (like the default of 15m) and a fast refresh interval (1-3s) the edges we get back from the API are not consistent, so you can see the map changing frequently.

See this video for an example: https://www.dropbox.com/s/jsq2bffxdw61xhu/77132.mov?dl=0

Fixes #73156.
Fixes #76936.

@smith smith added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 labels Sep 10, 2020
@smith smith marked this pull request as ready for review September 10, 2020 23:16
@smith smith requested a review from a team as a code owner September 10, 2020 23:16
@smith
Copy link
Contributor Author

smith commented Sep 14, 2020

@elasticmachine merge upstream

@botelastic botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Sep 14, 2020
@elasticmachine
Copy link
Contributor

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

@smith
Copy link
Contributor Author

smith commented Sep 14, 2020

retest

@smith
Copy link
Contributor Author

smith commented Sep 15, 2020

@elasticmachine merge upstream

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

Good cleanup 👍

};
}

export function useCytoscapeEventHandlers({
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to add a simple test for this hook?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
apm 1256 +3 1253

async chunks size

id value diff baseline
apm 5.0MB +4.9KB 4.9MB

History

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

@smith smith merged commit f98bed4 into elastic:master Sep 15, 2020
@smith smith deleted the nls/map-reset branch September 15, 2020 17:34
smith added a commit to smith/kibana that referenced this pull request Sep 15, 2020
These changes make the map work better with auto-refresh and with dragged nodes.

Extract all event handling out of the Cytoscape component into a hook.
Use React.memo to only render when the list of element ids has changed.
Only do a fit on the layout if we're going from 0 to more than 0 elements.
Instead of removing all the nodes on rerender, only remove the ones that aren't in the new list.
Trigger a custom:data event instead of data when we receive fetched data. Before we triggered a data event which would trigger a layout if you called data() on an element.
Don't trigger a deselect when we get new data, so popovers stay open when we get new data.
Animate the layout on changes.
When we do a layout, exclude selected nodes and nodes that have been dragged.
When we set the time range to something low (like the default of 15m) and a fast refresh interval (1-3s) the edges we get back from the API are not consistent, so you can see the map changing frequently.

See this video for an example: https://www.dropbox.com/s/jsq2bffxdw61xhu/77132.mov?dl=0

Fixes elastic#73156.
Fixes elastic#76936.
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 16, 2020
…s-for-710

* 'master' of github.com:elastic/kibana: (95 commits)
  log request body in new ES client (elastic#77150)
  use `navigateToUrl` to navigate to recent nav links (elastic#77446)
  Move core config service to `kbn/config` package (elastic#76874)
  [UBI] Copy license to /licenses folder (elastic#77563)
  Skip flaky Events Viewer Cypress test
  [Lens] Remove dynamic names in telemetry fields (elastic#76988)
  [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366)
  [Enterprise Search] Add flag to restrict width of layout (elastic#77539)
  [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967)
  [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090)
  [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058)
  [@kbn/utils] Adds missing dependency (elastic#77536)
  Add the Enterprise Search logo to the Overview search result (elastic#77514)
  [Logs UI] [Metrics UI] Remove saved object field mappings (elastic#75482)
  [Security Solution][Exceptions] Exception modal bulk close option only closes alerts generated by same rule (elastic#77402)
  Adds @kbn/utils package (elastic#76518)
  Map layout changes (elastic#77132)
  [Security Solution] [Detections] EQL Rule Creation (elastic#76831)
  Adding test user to maps tests under documents source folder  (elastic#77245)
  Suppress error logs when clients connect over HTTP instead of HTTPS (elastic#77397)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/index.ts
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/phases/warm_phase.tsx
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 17, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 18, 2020
smith added a commit that referenced this pull request Sep 18, 2020
These changes make the map work better with auto-refresh and with dragged nodes.

Extract all event handling out of the Cytoscape component into a hook.
Use React.memo to only render when the list of element ids has changed.
Only do a fit on the layout if we're going from 0 to more than 0 elements.
Instead of removing all the nodes on rerender, only remove the ones that aren't in the new list.
Trigger a custom:data event instead of data when we receive fetched data. Before we triggered a data event which would trigger a layout if you called data() on an element.
Don't trigger a deselect when we get new data, so popovers stay open when we get new data.
Animate the layout on changes.
When we do a layout, exclude selected nodes and nodes that have been dragged.
When we set the time range to something low (like the default of 15m) and a fast refresh interval (1-3s) the edges we get back from the API are not consistent, so you can see the map changing frequently.

See this video for an example: https://www.dropbox.com/s/jsq2bffxdw61xhu/77132.mov?dl=0

Fixes #73156.
Fixes #76936.
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:APM - DEPRECATED Use Team:obs-ux-infra_services. v7.10.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[APM] Service maps: Refresh interval will reset the nodes that might have been manipulated [APM] Kibana auto-refresh resets the service map view

4 participants