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

fix dashboard grid item performs 2 DOM queries every render #199390

Merged
merged 15 commits into from
Nov 18, 2024

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Nov 7, 2024

Closes #199361

While investigating, I found that fetching DOM element with id app-fixed-viewport is a common pattern. I created the hook useAppFixedViewport to consolidate this logic into a single location. The hook only performs the DOM look-up on first render and then avoids the DOM look-up on each additional render.

@nreese
Copy link
Contributor Author

nreese commented Nov 7, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Nov 7, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Nov 7, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Nov 7, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Nov 8, 2024

/ci

@nreese nreese marked this pull request as ready for review November 8, 2024 13:45
@nreese nreese requested review from a team as code owners November 8, 2024 13:45
@nreese nreese added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas release_note:skip Skip the PR/issue when compiling release notes v9.0.0 backport:version Backport to applied version labels v8.17.0 labels Nov 8, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@nreese
Copy link
Contributor Author

nreese commented Nov 11, 2024

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-management Observability Management User Experience Team labels Nov 11, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

TinaHeiligers

This comment was marked as outdated.

@TinaHeiligers TinaHeiligers self-requested a review November 14, 2024 17:57
@nreese nreese requested a review from TinaHeiligers November 14, 2024 18:39
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

useAppFixedViewport is an implementation and needs to be declared in the core-rendering-browser-internal package.

See suggested fixes.

@TinaHeiligers TinaHeiligers self-requested a review November 14, 2024 19:25
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

The changes don't strictly follow Core's packages conventions.
I won't hold the PR up though, as we can follow up with these.

@TinaHeiligers TinaHeiligers requested review from afharo and removed request for afharo November 14, 2024 19:29
@nreese nreese dismissed afharo’s stale review November 14, 2024 19:52

Core team review provided by Tina. Resolved afharo's concerns but he is out of office

@nreese
Copy link
Contributor Author

nreese commented Nov 14, 2024

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Nov 15, 2024

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Nov 18, 2024

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 18, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: d96a130
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-199390-d96a130bccae

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
core 445 448 +3
dashboard 687 690 +3
expressionPartitionVis 197 200 +3
expressionXY 258 261 +3
synthetics 1037 1040 +3
uptime 588 591 +3
total +18

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/core-rendering-browser - 2 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 647.0KB 647.1KB +165.0B
expressionPartitionVis 35.5KB 35.6KB +74.0B
expressionXY 127.6KB 127.7KB +59.0B
synthetics 1.1MB 1.1MB +26.0B
uptime 467.4KB 467.4KB +26.0B
total +350.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
dashboard 52.0KB 52.0KB +2.0B
Unknown metric groups

API count

id before after diff
@kbn/core-rendering-browser - 2 +2

History

@nreese nreese merged commit 9f54503 into elastic:main Nov 18, 2024
42 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11898803830

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

You might need to backport the following PRs to 8.x:
- [Security solution] Assistant package + plugin dead code removal (#200235)
- [APM] Migrate time_range_metadata test to deployment agnostic (#200146)
- [Console]: incorrect position of console tour #198389 (#198636)
- Update roles in serverless for value lists (#200128)
- [Inventory] Remove open in Discover button (#200574)
- [Entity Analytics] API changes for right placement of deleting the old component template (#199734)

Manual backport

To create the backport manually run:

node scripts/backport --pr 199390

Questions ?

Please refer to the Backport tool documentation

@nreese
Copy link
Contributor Author

nreese commented Nov 18, 2024

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@nreese
Copy link
Contributor Author

nreese commented Nov 18, 2024

8.x #200648

nreese added a commit to nreese/kibana that referenced this pull request Nov 18, 2024
…199390)

Closes elastic#199361

While investigating, I found that fetching DOM element with id
`app-fixed-viewport` is a common pattern. I created the hook
`useAppFixedViewport` to consolidate this logic into a single location.
The hook only performs the DOM look-up on first render and then avoids
the DOM look-up on each additional render.

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 9f54503)

# Conflicts:
#	.github/CODEOWNERS
nreese added a commit that referenced this pull request Nov 18, 2024
…99390) (#200648)

# Backport

This will backport the following commits from `main` to `8.x`:
- [fix dashboard grid item performs 2 DOM queries every render
(#199390)](#199390)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Nathan
Reese","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-18T18:32:53Z","message":"fix
dashboard grid item performs 2 DOM queries every render
(#199390)\n\nCloses
https://github.com/elastic/kibana/issues/199361\r\n\r\nWhile
investigating, I found that fetching DOM element with
id\r\n`app-fixed-viewport` is a common pattern. I created the
hook\r\n`useAppFixedViewport` to consolidate this logic into a single
location.\r\nThe hook only performs the DOM look-up on first render and
then avoids\r\nthe DOM look-up on each additional
render.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"9f545039ab42e95fd1d3d0518da4df6a8d040177","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Presentation","release_note:skip","v9.0.0","ci:project-deploy-observability","Team:obs-ux-management","backport:version","v8.17.0"],"number":199390,"url":"https://github.com/elastic/kibana/pull/199390","mergeCommit":{"message":"fix
dashboard grid item performs 2 DOM queries every render
(#199390)\n\nCloses
https://github.com/elastic/kibana/issues/199361\r\n\r\nWhile
investigating, I found that fetching DOM element with
id\r\n`app-fixed-viewport` is a common pattern. I created the
hook\r\n`useAppFixedViewport` to consolidate this logic into a single
location.\r\nThe hook only performs the DOM look-up on first render and
then avoids\r\nthe DOM look-up on each additional
render.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"9f545039ab42e95fd1d3d0518da4df6a8d040177"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/199390","number":199390,"mergeCommit":{"message":"fix
dashboard grid item performs 2 DOM queries every render
(#199390)\n\nCloses
https://github.com/elastic/kibana/issues/199361\r\n\r\nWhile
investigating, I found that fetching DOM element with
id\r\n`app-fixed-viewport` is a common pattern. I created the
hook\r\n`useAppFixedViewport` to consolidate this logic into a single
location.\r\nThe hook only performs the DOM look-up on first render and
then avoids\r\nthe DOM look-up on each additional
render.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"9f545039ab42e95fd1d3d0518da4df6a8d040177"}},{"branch":"8.x","label":"v8.17.0","labelRegex":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
…199390)

Closes elastic#199361

While investigating, I found that fetching DOM element with id
`app-fixed-viewport` is a common pattern. I created the hook
`useAppFixedViewport` to consolidate this logic into a single location.
The hook only performs the DOM look-up on first render and then avoids
the DOM look-up on each additional render.

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dashboard][performance] dashboard grid item performs 2 DOM queries every render
8 participants