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

[Unified Field List] Fix issue where Unified Field List field popover gets cut off #195147

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

davismcphee
Copy link
Contributor

@davismcphee davismcphee commented Oct 4, 2024

Summary

This PR fixes an issue where the Unified Field List field popover can get cut off if its contents exceed the view height. Now, instead of cutting off the popover, we limit the content height to 90vh and make the main section scrollable.

Before (from #194313 test failure):
image

After:
scroll

Flaky test runs:

Fixes #194313.
Fixes #193934.
Fixes #193781.

Checklist

For maintainers

@davismcphee davismcphee added release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) backport:prev-major Backport to (8.x, 8.17, 8.16) the previous major branch and other branches in development labels Oct 4, 2024
@davismcphee davismcphee self-assigned this Oct 4, 2024
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

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/unified-field-list 285 286 +1

Async chunks

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

id before after diff
aiops 552.7KB 553.1KB +361.0B
apm 3.4MB 3.4MB +361.0B
discover 820.0KB 820.5KB +473.0B
lens 1.5MB 1.5MB +541.0B
securitySolution 20.6MB 20.6MB +473.0B
slo 855.4KB 855.8KB +473.0B
total +2.6KB
Unknown metric groups

API count

id before after diff
@kbn/unified-field-list 314 315 +1

ESLint disabled line counts

id before after diff
@kbn/unified-field-list 17 18 +1

Total ESLint disabled count

id before after diff
@kbn/unified-field-list 17 18 +1

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

cc @davismcphee

@davismcphee davismcphee marked this pull request as ready for review October 4, 2024 23:15
@davismcphee davismcphee requested review from a team as code owners October 4, 2024 23:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7098

[❌] x-pack/test_serverless/functional/test_suites/observability/common_configs/config.group6.ts: 25/50 tests passed.
[❌] x-pack/test_serverless/functional/test_suites/search/common_configs/config.group6.ts: 49/50 tests passed.
[✅] x-pack/test_serverless/functional/test_suites/security/common_configs/config.group6.ts: 50/50 tests passed.

see run history

@davismcphee
Copy link
Contributor Author

The failure from the flaky test run seems to be caused by the issue resolved in #195154, not the one that caused these failures:
image

{content ? <EuiPopoverTitle>{header}</EuiPopoverTitle> : header}
</EuiFlexItem>
)}
{content ? <EuiFlexItem css={{ overflowY: 'auto' }}>{content}</EuiFlexItem> : content}
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a small issue with the scroll indicator being visible during stats loading:

Oct-07-2024 10-19-12

Does it need a min height?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I fixed it with padding and negative margins, and also added EUI scroll styles: f63fe9c.

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #11 / Header rendering it renders the data providers when show is true

Metrics [docs]

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/unified-field-list 285 286 +1

Async chunks

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

id before after diff
aiops 553.7KB 554.1KB +447.0B
apm 3.4MB 3.4MB +449.0B
discover 820.0KB 820.6KB +562.0B
lens 1.5MB 1.5MB +621.0B
securitySolution 20.6MB 20.6MB +562.0B
slo 855.4KB 855.9KB +562.0B
total +3.1KB
Unknown metric groups

API count

id before after diff
@kbn/unified-field-list 314 315 +1

ESLint disabled line counts

id before after diff
@kbn/unified-field-list 17 18 +1

Total ESLint disabled count

id before after diff
@kbn/unified-field-list 17 18 +1

History

cc @davismcphee

Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 👍

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

LGTM 👌🏼

@davismcphee davismcphee merged commit 26d5634 into elastic:main Oct 8, 2024
21 checks passed
@davismcphee davismcphee deleted the fix-field-popover-cutoff branch October 8, 2024 21:43
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.15, 8.x

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 8, 2024
… gets cut off (elastic#195147)

## Summary

This PR fixes an issue where the Unified Field List field popover can
get cut off if its contents exceed the view height. Now, instead of
cutting off the popover, we limit the content height to `90vh` and make
the main section scrollable.

Before (from elastic#194313 test failure):

![image](https://github.com/user-attachments/assets/5927a899-a18a-4431-bd1d-6bb2682cd004)

After:

![scroll](https://github.com/user-attachments/assets/5071a52b-fbf4-4d05-96de-61858d9e5598)

Flaky test runs:
-
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7098

Fixes elastic#194313.
Fixes elastic#193934.
Fixes elastic#193781.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit 26d5634)
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

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

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

Manual backport

To create the backport manually run:

node scripts/backport --pr 195147

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 8, 2024
…opover gets cut off (#195147) (#195512)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Unified Field List] Fix issue where Unified Field List field popover
gets cut off (#195147)](#195147)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Davis
McPhee","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-08T21:43:52Z","message":"[Unified
Field List] Fix issue where Unified Field List field popover gets cut
off (#195147)\n\n## Summary\r\n\r\nThis PR fixes an issue where the
Unified Field List field popover can\r\nget cut off if its contents
exceed the view height. Now, instead of\r\ncutting off the popover, we
limit the content height to `90vh` and make\r\nthe main section
scrollable.\r\n\r\nBefore (from #194313 test
failure):\r\n\r\n![image](https://github.com/user-attachments/assets/5927a899-a18a-4431-bd1d-6bb2682cd004)\r\n\r\nAfter:\r\n\r\n![scroll](https://github.com/user-attachments/assets/5071a52b-fbf4-4d05-96de-61858d9e5598)\r\n\r\nFlaky
test
runs:\r\n-\r\nhttps://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7098\r\n\r\nFixes
#194313.\r\nFixes #193934.\r\nFixes #193781.\r\n\r\n###
Checklist\r\n\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [ ] [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] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is
usable by keyboard only (learn more\r\nabout [keyboard
accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI
touched in this PR does not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[ ] If a plugin configuration key changed, check if it needs to
be\r\nallowlisted in the cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[x] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[ ] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n###
For maintainers\r\n\r\n- [ ] This was checked for breaking API changes
and was
[labeled\r\nappropriately](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
<[email protected]>","sha":"26d5634b23fc76d7a87aaba53892dabb31866d54","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","v9.0.0","Team:DataDiscovery","backport:prev-minor","backport:prev-major"],"title":"[Unified
Field List] Fix issue where Unified Field List field popover gets cut
off","number":195147,"url":"https://github.com/elastic/kibana/pull/195147","mergeCommit":{"message":"[Unified
Field List] Fix issue where Unified Field List field popover gets cut
off (#195147)\n\n## Summary\r\n\r\nThis PR fixes an issue where the
Unified Field List field popover can\r\nget cut off if its contents
exceed the view height. Now, instead of\r\ncutting off the popover, we
limit the content height to `90vh` and make\r\nthe main section
scrollable.\r\n\r\nBefore (from #194313 test
failure):\r\n\r\n![image](https://github.com/user-attachments/assets/5927a899-a18a-4431-bd1d-6bb2682cd004)\r\n\r\nAfter:\r\n\r\n![scroll](https://github.com/user-attachments/assets/5071a52b-fbf4-4d05-96de-61858d9e5598)\r\n\r\nFlaky
test
runs:\r\n-\r\nhttps://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7098\r\n\r\nFixes
#194313.\r\nFixes #193934.\r\nFixes #193781.\r\n\r\n###
Checklist\r\n\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [ ] [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] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is
usable by keyboard only (learn more\r\nabout [keyboard
accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI
touched in this PR does not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[ ] If a plugin configuration key changed, check if it needs to
be\r\nallowlisted in the cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[x] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[ ] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n###
For maintainers\r\n\r\n- [ ] This was checked for breaking API changes
and was
[labeled\r\nappropriately](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
<[email protected]>","sha":"26d5634b23fc76d7a87aaba53892dabb31866d54"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195147","number":195147,"mergeCommit":{"message":"[Unified
Field List] Fix issue where Unified Field List field popover gets cut
off (#195147)\n\n## Summary\r\n\r\nThis PR fixes an issue where the
Unified Field List field popover can\r\nget cut off if its contents
exceed the view height. Now, instead of\r\ncutting off the popover, we
limit the content height to `90vh` and make\r\nthe main section
scrollable.\r\n\r\nBefore (from #194313 test
failure):\r\n\r\n![image](https://github.com/user-attachments/assets/5927a899-a18a-4431-bd1d-6bb2682cd004)\r\n\r\nAfter:\r\n\r\n![scroll](https://github.com/user-attachments/assets/5071a52b-fbf4-4d05-96de-61858d9e5598)\r\n\r\nFlaky
test
runs:\r\n-\r\nhttps://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7098\r\n\r\nFixes
#194313.\r\nFixes #193934.\r\nFixes #193781.\r\n\r\n###
Checklist\r\n\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [ ] [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] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is
usable by keyboard only (learn more\r\nabout [keyboard
accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI
touched in this PR does not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[ ] If a plugin configuration key changed, check if it needs to
be\r\nallowlisted in the cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[x] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[ ] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n###
For maintainers\r\n\r\n- [ ] This was checked for breaking API changes
and was
[labeled\r\nappropriately](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
<[email protected]>","sha":"26d5634b23fc76d7a87aaba53892dabb31866d54"}}]}]
BACKPORT-->

Co-authored-by: Davis McPhee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment