[kbn-grid-layout] Cleanup memoization and styling#210285
Merged
Heenawter merged 3 commits intoelastic:mainfrom Feb 10, 2025
Merged
[kbn-grid-layout] Cleanup memoization and styling#210285Heenawter merged 3 commits intoelastic:mainfrom
Heenawter merged 3 commits intoelastic:mainfrom
Conversation
ea0e84d to
e7ec22e
Compare
Contributor
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
mbondyra
approved these changes
Feb 10, 2025
Contributor
💚 Build Succeeded
Metrics [docs]Async chunks
History
cc @Heenawter |
nickpeihl
approved these changes
Feb 10, 2025
Contributor
nickpeihl
left a comment
There was a problem hiding this comment.
lgtm! discussed offline with @Heenawter
code review only
Contributor
|
Starting backport for target branches: 8.x |
Contributor
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
Heenawter
added a commit
to Heenawter/kibana
that referenced
this pull request
Feb 10, 2025
## Summary This PR cleans up the `kbn-grid-layout` code in two ways: 1. Rather than memoizing components in their parents, I swapped to using `React.memo` for all components, which accomplishes the same behaviour in a slightly cleaner way. 2. I moved all Emotion style definitions **outside** of the React components so that we no longer have to re-parse the CSS string on every render (see [this comment](elastic/eui#6828 (comment))). ### Checklist - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 219f31a) # Conflicts: # src/platform/packages/private/kbn-grid-layout/grid/grid_panel/resize_handle.tsx
Heenawter
added a commit
that referenced
this pull request
Feb 11, 2025
…10466) # Backport This will backport the following commits from `main` to `8.x`: - [[kbn-grid-layout] Cleanup memoization and styling (#210285)](#210285) <!--- Backport version: 9.6.4 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Hannah Mudge","email":"Heenawter@users.noreply.github.com"},"sourceCommit":{"committedDate":"2025-02-05T22:18:04Z","message":"[Dashboard] Presentation panel refactor (#207275)\n\nCloses https://github.com/elastic/kibana/issues/206686\r\nCloses https://github.com/elastic/kibana/issues/197897\r\nPart of https://github.com/elastic/kibana/issues/207852\r\n\r\n## Summary\r\n\r\nThis PR is a major refactor of the `PresentationPanel` component,\r\nincluding an overhaul of the hover action and panel title components.\r\nSome notable highlights include:\r\n- All styles in the `PresentationPanel` component were moved from SASS\r\nto Emotion\r\n- The over-complicated logic to combine hover actions when the panel\r\nshrinks was removed in favour of CSS, driven by a [container\r\nquery](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_containment/Container_queries)\r\nRemoving the `updateCombineHoverActions` function (which was defined in\r\na React component and not memoized) also made a difference in\r\nperformance when dragging:\r\n \r\n | Before | After |\r\n |--------|--------|\r\n|\r\n\r\n|\r\n\r\n|\r\n \r\n\r\n- The over-complicated logic defined in\r\n`usePresentationPanelTitleClickHandle`, which was meant to ignore the\r\n`onClick` that would trigger after a panel was dragged, was converted to\r\n2 lines of CSS\r\n\r\n### Small usability improvements\r\n\r\nThis PR also includes a few small usability improvements, such as:\r\n\r\n- Ensuring that only the **first** row of hover actions overlaps with\r\nthe Dashboard's sticky top navigation bar, and this only happens when\r\nthe dashboard has no controls. This results in much better behaviour in\r\nmost scenarios:\r\n \r\n | Before | After |\r\n |--------|--------|\r\n| \r\n| \r\n|\r\n\r\n- Adding a small delay for hiding the hover actions on mouse leave,\r\nwhich makes it a lot easier to grab the drag handle:\r\n\r\n | Before | After |\r\n |--------|--------|\r\n| \r\n| \r\n|\r\n\r\n- Preventing the resize handle from overlapping Dashboard's stick top\r\nnavigation:\r\n\r\n | Before | After |\r\n |--------|--------|\r\n| \r\n| \r\n|\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"c35698bcf81314f833467fde2d3c14785b83c1ad","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["performance","Team:Presentation","loe:medium","technical debt","release_note:skip","impact:high","backport:version","v9.1.0","v8.19.0"],"title":"[Dashboard] Presentation panel refactor","number":207275,"url":"https://github.com/elastic/kibana/pull/207275","mergeCommit":{"message":"[Dashboard] Presentation panel refactor (#207275)\n\nCloses https://github.com/elastic/kibana/issues/206686\r\nCloses https://github.com/elastic/kibana/issues/197897\r\nPart of https://github.com/elastic/kibana/issues/207852\r\n\r\n## Summary\r\n\r\nThis PR is a major refactor of the `PresentationPanel` component,\r\nincluding an overhaul of the hover action and panel title components.\r\nSome notable highlights include:\r\n- All styles in the `PresentationPanel` component were moved from SASS\r\nto Emotion\r\n- The over-complicated logic to combine hover actions when the panel\r\nshrinks was removed in favour of CSS, driven by a [container\r\nquery](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_containment/Container_queries)\r\nRemoving the `updateCombineHoverActions` function (which was defined in\r\na React component and not memoized) also made a difference in\r\nperformance when dragging:\r\n \r\n | Before | After |\r\n |--------|--------|\r\n|\r\n\r\n|\r\n\r\n|\r\n \r\n\r\n- The over-complicated logic defined in\r\n`usePresentationPanelTitleClickHandle`, which was meant to ignore the\r\n`onClick` that would trigger after a panel was dragged, was converted to\r\n2 lines of CSS\r\n\r\n### Small usability improvements\r\n\r\nThis PR also includes a few small usability improvements, such as:\r\n\r\n- Ensuring that only the **first** row of hover actions overlaps with\r\nthe Dashboard's sticky top navigation bar, and this only happens when\r\nthe dashboard has no controls. This results in much better behaviour in\r\nmost scenarios:\r\n \r\n | Before | After |\r\n |--------|--------|\r\n| \r\n| \r\n|\r\n\r\n- Adding a small delay for hiding the hover actions on mouse leave,\r\nwhich makes it a lot easier to grab the drag handle:\r\n\r\n | Before | After |\r\n |--------|--------|\r\n| \r\n| \r\n|\r\n\r\n- Preventing the resize handle from overlapping Dashboard's stick top\r\nnavigation:\r\n\r\n | Before | After |\r\n |--------|--------|\r\n| \r\n| \r\n|\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"c35698bcf81314f833467fde2d3c14785b83c1ad"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/207275","number":207275,"mergeCommit":{"message":"[Dashboard] Presentation panel refactor (#207275)\n\nCloses https://github.com/elastic/kibana/issues/206686\r\nCloses https://github.com/elastic/kibana/issues/197897\r\nPart of https://github.com/elastic/kibana/issues/207852\r\n\r\n## Summary\r\n\r\nThis PR is a major refactor of the `PresentationPanel` component,\r\nincluding an overhaul of the hover action and panel title components.\r\nSome notable highlights include:\r\n- All styles in the `PresentationPanel` component were moved from SASS\r\nto Emotion\r\n- The over-complicated logic to combine hover actions when the panel\r\nshrinks was removed in favour of CSS, driven by a [container\r\nquery](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_containment/Container_queries)\r\nRemoving the `updateCombineHoverActions` function (which was defined in\r\na React component and not memoized) also made a difference in\r\nperformance when dragging:\r\n \r\n | Before | After |\r\n |--------|--------|\r\n|\r\n\r\n|\r\n\r\n|\r\n \r\n\r\n- The over-complicated logic defined in\r\n`usePresentationPanelTitleClickHandle`, which was meant to ignore the\r\n`onClick` that would trigger after a panel was dragged, was converted to\r\n2 lines of CSS\r\n\r\n### Small usability improvements\r\n\r\nThis PR also includes a few small usability improvements, such as:\r\n\r\n- Ensuring that only the **first** row of hover actions overlaps with\r\nthe Dashboard's sticky top navigation bar, and this only happens when\r\nthe dashboard has no controls. This results in much better behaviour in\r\nmost scenarios:\r\n \r\n | Before | After |\r\n |--------|--------|\r\n| \r\n| \r\n|\r\n\r\n- Adding a small delay for hiding the hover actions on mouse leave,\r\nwhich makes it a lot easier to grab the drag handle:\r\n\r\n | Before | After |\r\n |--------|--------|\r\n| \r\n| \r\n|\r\n\r\n- Preventing the resize handle from overlapping Dashboard's stick top\r\nnavigation:\r\n\r\n | Before | After |\r\n |--------|--------|\r\n| \r\n| \r\n|\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"c35698bcf81314f833467fde2d3c14785b83c1ad"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
1 task
Heenawter
added a commit
that referenced
this pull request
Feb 25, 2025
## Summary This PR fixes a broken style introduced in #210285 where, when rewriting the single column styles from strings to objects, I accidentally set the property `gridTemplateAreas` instead of `gridTemplateColumns` - this resulted in the single column / mobile view being broken: | Before | After | |--------|--------| |  |  | ### Checklist - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Feb 25, 2025
## Summary This PR fixes a broken style introduced in elastic#210285 where, when rewriting the single column styles from strings to objects, I accidentally set the property `gridTemplateAreas` instead of `gridTemplateColumns` - this resulted in the single column / mobile view being broken: | Before | After | |--------|--------| |  |  | ### Checklist - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 464a471)
kibanamachine
added a commit
that referenced
this pull request
Feb 25, 2025
# Backport This will backport the following commits from `main` to `8.x`: - [[kbn-grid-layout] Fix mobile styles (#212312)](#212312) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Hannah Mudge","email":"Heenawter@users.noreply.github.com"},"sourceCommit":{"committedDate":"2025-02-25T16:26:26Z","message":"[kbn-grid-layout] Fix mobile styles (#212312)\n\n## Summary\n\nThis PR fixes a broken style introduced in\nhttps://github.com//pull/210285 where, when rewriting the\nsingle column styles from strings to objects, I accidentally set the\nproperty `gridTemplateAreas` instead of `gridTemplateColumns` - this\nresulted in the single column / mobile view being broken:\n\n| Before | After |\n|--------|--------|\n|\n\n|\n\n|\n\n\n### Checklist\n\n- [x] The PR description includes the appropriate Release Notes section,\nand the correct `release_note:*` label is applied per the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"464a4714468fa70552e4b110af5fdb17a3e44161","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Feature:Dashboard","Team:Presentation","loe:small","release_note:skip","impact:critical","backport:version","v9.1.0","v8.19.0"],"title":"[kbn-grid-layout] Fix mobile styles","number":212312,"url":"https://github.com/elastic/kibana/pull/212312","mergeCommit":{"message":"[kbn-grid-layout] Fix mobile styles (#212312)\n\n## Summary\n\nThis PR fixes a broken style introduced in\nhttps://github.com//pull/210285 where, when rewriting the\nsingle column styles from strings to objects, I accidentally set the\nproperty `gridTemplateAreas` instead of `gridTemplateColumns` - this\nresulted in the single column / mobile view being broken:\n\n| Before | After |\n|--------|--------|\n|\n\n|\n\n|\n\n\n### Checklist\n\n- [x] The PR description includes the appropriate Release Notes section,\nand the correct `release_note:*` label is applied per the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"464a4714468fa70552e4b110af5fdb17a3e44161"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/212312","number":212312,"mergeCommit":{"message":"[kbn-grid-layout] Fix mobile styles (#212312)\n\n## Summary\n\nThis PR fixes a broken style introduced in\nhttps://github.com//pull/210285 where, when rewriting the\nsingle column styles from strings to objects, I accidentally set the\nproperty `gridTemplateAreas` instead of `gridTemplateColumns` - this\nresulted in the single column / mobile view being broken:\n\n| Before | After |\n|--------|--------|\n|\n\n|\n\n|\n\n\n### Checklist\n\n- [x] The PR description includes the appropriate Release Notes section,\nand the correct `release_note:*` label is applied per the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"464a4714468fa70552e4b110af5fdb17a3e44161"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com>
JoseLuisGJ
pushed a commit
to JoseLuisGJ/kibana
that referenced
this pull request
Feb 27, 2025
## Summary This PR fixes a broken style introduced in elastic#210285 where, when rewriting the single column styles from strings to objects, I accidentally set the property `gridTemplateAreas` instead of `gridTemplateColumns` - this resulted in the single column / mobile view being broken: | Before | After | |--------|--------| |  |  | ### Checklist - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
SoniaSanzV
pushed a commit
to SoniaSanzV/kibana
that referenced
this pull request
Mar 4, 2025
…12410) # Backport This will backport the following commits from `main` to `8.x`: - [[kbn-grid-layout] Fix mobile styles (elastic#212312)](elastic#212312) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Hannah Mudge","email":"Heenawter@users.noreply.github.com"},"sourceCommit":{"committedDate":"2025-02-25T16:26:26Z","message":"[kbn-grid-layout] Fix mobile styles (elastic#212312)\n\n## Summary\n\nThis PR fixes a broken style introduced in\nhttps://github.com/elastic/pull/210285 where, when rewriting the\nsingle column styles from strings to objects, I accidentally set the\nproperty `gridTemplateAreas` instead of `gridTemplateColumns` - this\nresulted in the single column / mobile view being broken:\n\n| Before | After |\n|--------|--------|\n|\n\n|\n\n|\n\n\n### Checklist\n\n- [x] The PR description includes the appropriate Release Notes section,\nand the correct `release_note:*` label is applied per the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"464a4714468fa70552e4b110af5fdb17a3e44161","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Feature:Dashboard","Team:Presentation","loe:small","release_note:skip","impact:critical","backport:version","v9.1.0","v8.19.0"],"title":"[kbn-grid-layout] Fix mobile styles","number":212312,"url":"https://github.com/elastic/kibana/pull/212312","mergeCommit":{"message":"[kbn-grid-layout] Fix mobile styles (elastic#212312)\n\n## Summary\n\nThis PR fixes a broken style introduced in\nhttps://github.com/elastic/pull/210285 where, when rewriting the\nsingle column styles from strings to objects, I accidentally set the\nproperty `gridTemplateAreas` instead of `gridTemplateColumns` - this\nresulted in the single column / mobile view being broken:\n\n| Before | After |\n|--------|--------|\n|\n\n|\n\n|\n\n\n### Checklist\n\n- [x] The PR description includes the appropriate Release Notes section,\nand the correct `release_note:*` label is applied per the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"464a4714468fa70552e4b110af5fdb17a3e44161"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/212312","number":212312,"mergeCommit":{"message":"[kbn-grid-layout] Fix mobile styles (elastic#212312)\n\n## Summary\n\nThis PR fixes a broken style introduced in\nhttps://github.com/elastic/pull/210285 where, when rewriting the\nsingle column styles from strings to objects, I accidentally set the\nproperty `gridTemplateAreas` instead of `gridTemplateColumns` - this\nresulted in the single column / mobile view being broken:\n\n| Before | After |\n|--------|--------|\n|\n\n|\n\n|\n\n\n### Checklist\n\n- [x] The PR description includes the appropriate Release Notes section,\nand the correct `release_note:*` label is applied per the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"464a4714468fa70552e4b110af5fdb17a3e44161"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com>
CAWilson94
pushed a commit
to CAWilson94/kibana
that referenced
this pull request
Mar 22, 2025
## Summary This PR fixes a broken style introduced in elastic#210285 where, when rewriting the single column styles from strings to objects, I accidentally set the property `gridTemplateAreas` instead of `gridTemplateColumns` - this resulted in the single column / mobile view being broken: | Before | After | |--------|--------| |  |  | ### Checklist - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR cleans up the
kbn-grid-layoutcode in two ways:React.memofor all components, which accomplishes the same behaviour in a slightly cleaner way.Checklist
release_note:*label is applied per the guidelines