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

[APM] Fit service map to container #56336

Merged
merged 1 commit into from
Jan 30, 2020
Merged

[APM] Fit service map to container #56336

merged 1 commit into from
Jan 30, 2020

Conversation

smith
Copy link
Contributor

@smith smith commented Jan 29, 2020 •
edited
Loading

  • Set the height of the service map dynamically
  • Move the loading overlay from the outside to the inside of the Cytoscape container
  • Remove the EUI spacer from the Home and ServiceDetailTabs components and add it to the individual components that use them
  • Change the main element to a div role=main because the main element is not supported in IE 11

Fixes #54283

@smith smith added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 apm:service-maps Service Map feature in APM labels Jan 29, 2020
@smith smith requested a review from a team as a code owner January 29, 2020 19:45
ogupte
ogupte reviewed Jan 29, 2020
View reviewed changes
x-pack/legacy/plugins/apm/public/components/app/ServiceMap/useRefHeight.ts Outdated
const approximateGuessOfDefaultTopOffset = 230;
const topOffset =
ref.current?.getBoundingClientRect()?.top ??
approximateGuessOfDefaultTopOffset;
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make a noticeable difference if there is no default value here? If height defaults to undefined or null, then gets set as a prop for the Cytoscape component, would it just render with the default height anyways?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't make much of a difference, but on the initial load the height is the full window height, so there's a scrollbar there until we get the ref and the top offset.

I'm changing this so we just return 0 for the height if we don't yet have the offset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in f9a1448.

@smith
[APM] Fit service map to container
f9a1448 
* Set the height of the service map dynamically
* Move the loading overlay from the outside to the inside of the Cytoscape container
* Remove the EUI spacer from the Home and ServiceDetailTabs components and add it to the individual components that use them
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

ogupte
ogupte approved these changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@ogupte ogupte left a comment

Choose a reason for hiding this comment

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

Looks good!

@smith smith merged commit 4084be7 into elastic:master Jan 30, 2020
@smith smith deleted the nls/fit-fixes branch January 30, 2020 04:23
@smith smith mentioned this pull request Jan 30, 2020
Closed
smith added a commit to smith/kibana that referenced this pull request Jan 31, 2020
@smith
[APM] Fit service map to container (elastic#56336)
73a6ce4 
* Set the height of the service map dynamically
* Move the loading overlay from the outside to the inside of the Cytoscape container
* Remove the EUI spacer from the Home and ServiceDetailTabs components and add it to the individual components that use them
@smith smith mentioned this pull request Jan 31, 2020
Merged
smith added a commit that referenced this pull request Jan 31, 2020
@smith
[APM] Fit service map to container (#56336) (#56463)
4dbe14d 
* Set the height of the service map dynamically
* Move the loading overlay from the outside to the inside of the Cytoscape container
* Remove the EUI spacer from the Home and ServiceDetailTabs components and add it to the individual components that use them
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 31, 2020
@jloleysens
Merge branch '7.x' of github.com:elastic/kibana into backport/7.x/pr-…
f8afb1b 
…56356

* '7.x' of github.com:elastic/kibana: (23 commits)
  Fix setting filters without $store value (elastic#56304) (elastic#56475)
  [ML] Fix Data Visualizer responsive layout  (elastic#56372) (elastic#56472)
  [ML] conditional rison encoding for query params (elastic#56380) (elastic#56469)
  kuery_autocomplete -> convert remaining items to TS/Jest (elastic#56316) (elastic#56471)
  [APM] Fit service map to container (elastic#56336) (elastic#56463)
  Add animation to service map layout (elastic#56042) (elastic#56460)
  chore(NA): delete data/optimize with kbn clean (elastic#55890) (elastic#56422)
  [APM] Storybook support (elastic#54970) (elastic#56445)
  [DOCS] Updates example in Timelion doc (elastic#56444) (elastic#56454)
  [Logs UI] Fix Check for New Data button on empty indices screen (elastic#56239) (elastic#56320)
  [DOCS] Adds breaking changes for 7.6 (elastic#56437)
  [Monitoring] Change all configs to `monitoring.*` (elastic#56215) (elastic#56421)
  [skip-ci] Add example for migrating pre-handlers (elastic#56080) (elastic#56436)
  [7.x] System index templates can't be edited (elastic#55229) (elastic#56417)
  Add missing docker settings (elastic#56411)
  [Uptime] Use dynamic index pattern in Uptime (elastic#55446) (elastic#56386)
  fix edit rule for detections (elastic#56333) (elastic#56405)
  [Filter Bar] Remove flickering when opening filter bar popover (elastic#56222) (elastic#56385)
  [ILM] Index Lifecycle Policies show wrong unit in Kibana UI (elastic#55228) (elastic#55757)
  Move tsvb server to new platform (elastic#55310) (elastic#56394)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogupte ogupte ogupte approved these changes

Assignees
No one assigned
Labels
apm:service-maps Service Map feature in APM release_note:skip Skip the PR/issue when compiling release notes v7.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] [Service map] Canvas layout issues
3 participants
@smith @kibanamachine @ogupte