Conversation
2d94572 to
3d385ce
Compare
- replace event to make grid row cell visible - focus event now handled by a separate subcomponent, which enzyme simulate doesn't know about
- Cypress is now being flaky about these clicks
- flex was being weird, `height: auto` was needed instead of `100%` - use `alignIndicator="start"` to keep the drag handle nearish to the collapse button
+ rename hack - The behavior works fine now with the event listeners, but it's a bit smoother even with the overlay, so I think keeping it makes sense
Request from Davis - use useLatest to make callback gotcha clearer to Kibana devs
|
/ci |
|
@patrykkopycinski going to rebase latest to drop an unrelated commit as a heads up! |
- not sure why they're failing/timing out with the previous clicks, likely same Cypress race condition weirdness as previous commits
|
/ci |
| } | ||
| // The resize overlay makes it so that other mouse events (e.g. tooltip hovers, etc) | ||
| // don't occur when mouse dragging | ||
| const [isResizing, setIsResizing] = useState(false); |
There was a problem hiding this comment.
@davismcphee I'm hoping you can approve/review these changes to the package/Discover layout since we've been discussing this last week! 🙏 Please do let me know if you find any issues or aren't a huge fan of the renames, etc.
There was a problem hiding this comment.
The changes look and work great, thanks! The renames/cleanups look good too, and I like that the window listener approach means we no longer have to work around react-reverse-portal swallowing our events (but keeping the overlay for tooltips, etc. makes sense).
| </EuiResizablePanel> | ||
|
|
||
| <EuiResizableButton | ||
| alignIndicator="start" |
There was a problem hiding this comment.
@elastic/kibana-visualizations I noticed while doing a quick grep through Kibana for <EuiResizableButton> usages that this button was slightly broken in production due to flex display issues. I fixed this with a height .scss change and added this indicator alignment prop to match production.
| Before | After |
|---|---|
![]() |
![]() |
There was a problem hiding this comment.
Thanx Cee! I really appreciate it
stratoula
left a comment
There was a problem hiding this comment.
Vis team changes LGTM!
tomsonpl
left a comment
There was a problem hiding this comment.
Thanks for the changes, I love the dataGrid so I am very excited to see the improvements ❤️
Left a few questions :)
| cy.getBySel('dataGridHeaderCell-osquery.disk_bytes_written.number').click(); | ||
|
|
||
| cy.contains(/Hide column$/).click(); | ||
| cy.getBySel('dataGridColumnSelectorButton').click(); |
There was a problem hiding this comment.
So the current behavior doesn't require clicking the header first then 'Hide column' button ? But instead using a specific selector?
There was a problem hiding this comment.
Yes, see above - for some reason trying to click the header cell actions was timing out Cypress (I could reproduce this locally, although I honestly have no idea why it was happening) and causing Chrome Renderer crashes. So I opted to use the selector toolbar button instead. EUI already has its own component Cypress tests for hiding columns via the header actions buttons, so hopefully we would catch any regressions around that UX ourselves before they ever reach Kibana.
| // sorting | ||
| cy.getBySel('dataGridHeaderCell-osquery.egid').click(); | ||
|
|
||
| cy.getBySel('dataGridHeaderCellActionButton-osquery.egid').click({ force: true }); |
There was a problem hiding this comment.
Cypress was being really flaky about these clicks (I'm not totally sure why as they work just fine in production, I think it's either layout settling or rerenders due to how fast it "clicks" things). CI was consistently failing for me without the force: true. If you can get it working in the future with another approach (e.g. a wait or a real click?) feel free to refactor this in the future!
davismcphee
left a comment
There was a problem hiding this comment.
Tested locally, and the Data Discovery changes LGTM 👍 It's great that the border resize indicator is officially supported by EUI now.
| } | ||
| // The resize overlay makes it so that other mouse events (e.g. tooltip hovers, etc) | ||
| // don't occur when mouse dragging | ||
| const [isResizing, setIsResizing] = useState(false); |
There was a problem hiding this comment.
The changes look and work great, thanks! The renames/cleanups look good too, and I like that the window listener approach means we no longer have to work around react-reverse-portal swallowing our events (but keeping the overlay for tooltips, etc. makes sense).
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* main: (520 commits) Update Kibana code editor dependencies (#171720) [SLOs] Hide view in app in slo alerts table in slo details page (#175441) [api-docs] 2024-01-25 Daily api_docs build (#175502) [DOCS] Add buildkite links to doc preview comments (#175463) skip flaky suite (#175443) [Security Solution][Timeline] refactor timeline modal save timeline button (#175343) [RAM] Stack Management::Rules loses user selections when navigating back (#174954) [Security Solution][Timeline] refactor timeline modal attach to case button (#175163) Upgrade EUI to v92.1.1 (#174955) [Fleet]: Beta label is shown inconsistently while selecting proxy under Fleet settings. (#170634) [Cloud Security] Rules Combo Box filters Custom component (#175175) skip flaky suite (#175407) [Security Solution][Timeline] refactor timeline modal open timeline button (#175335) [Embedded Console] Introduce kbnSolutionNavOffset CSS variable (#175348) [Console] disable access to embedded console without dev tools capability (#175321) fix(x-pack/reporting): use FIPS-compliant ID generator `uuidv4` in Reporting plugin (#174809) [Security Solution] Data quality dashboard persistence (#173185) [RAM][Observability] Add alert fields table to Observability flyout (#174685) test: add missing await for connector table disappearance (#175430) [RAM][Maintenance Window] Fix maintenance window FE types and transforms (#173888) ...
…76030) ## Summary This PR reverts the `onResizeEnd` stable callback workaround introduced in #174955 to account for an EUI bug now that elastic/eui#7468 has been merged and Kibana was upgraded to v92.2.1 in #175849. ### 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 - [ ] [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) - [ ] 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)
…astic#176030) ## Summary This PR reverts the `onResizeEnd` stable callback workaround introduced in elastic#174955 to account for an EUI bug now that elastic/eui#7468 has been merged and Kibana was upgraded to v92.2.1 in elastic#175849. ### 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 - [ ] [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) - [ ] 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)
`v92.0.0-backport.0`⏩ `v92.1.1` --- ## [`v92.1.1`](https://github.com/elastic/eui/releases/v92.1.1) **Bug fixes** - Minor `EuiDataGrid` cell performance fixes ([elastic#7465](elastic/eui#7465)) ## [`v92.1.0`](https://github.com/elastic/eui/releases/v92.1.0) - Updated `EuiResizableButton` to allow customizing the `indicator` style with either `handle` (default) or `border` ([elastic#7455](elastic/eui#7455)) - Enhanced `EuiResizableContainer` to preserve the drag/resize event when the user's mouse leaves the parent container and re-enters ([elastic#7456](elastic/eui#7456)) **Bug fixes** - Fixed an `EuiTreeView` JSX Typescript error ([elastic#7452](elastic/eui#7452)) - Fixed a color console warning being generated by disabled `EuiStep`s ([elastic#7454](elastic/eui#7454)) **Accessibility** - `EuiDataGrid`'s keyboard/screenreader experience has been tweaked to be more consistent for varying complex data: ([elastic#7448](elastic/eui#7448)) - Headers are now always navigable by arrow key, regardless of whether the header cells contain interactive content - Non-expandable cells containing any amount of interactive content now must be entered via Enter or F2 keypress - Expandable cells continue to be toggled via Enter or F2 keypress - `EuiDataGrid` now provides a direct screen reader hint for Enter key behavior for expandable & interactive cells ([elastic#7448](elastic/eui#7448))
…astic#176030) ## Summary This PR reverts the `onResizeEnd` stable callback workaround introduced in elastic#174955 to account for an EUI bug now that elastic/eui#7468 has been merged and Kibana was upgraded to v92.2.1 in elastic#175849. ### 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 - [ ] [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) - [ ] 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)
…astic#176030) ## Summary This PR reverts the `onResizeEnd` stable callback workaround introduced in elastic#174955 to account for an EUI bug now that elastic/eui#7468 has been merged and Kibana was upgraded to v92.2.1 in elastic#175849. ### 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 - [ ] [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) - [ ] 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)


v92.0.0-backport.0⏩v92.1.1v92.1.1Bug fixes
EuiDataGridcell performance fixes (#7465)v92.1.0EuiResizableButtonto allow customizing theindicatorstyle with eitherhandle(default) orborder(#7455)EuiResizableContainerto preserve the drag/resize event when the user's mouse leaves the parent container and re-enters (#7456)Bug fixes
EuiTreeViewJSX Typescript error (#7452)EuiSteps (#7454)Accessibility
EuiDataGrid's keyboard/screenreader experience has been tweaked to be more consistent for varying complex data: (#7448)EuiDataGridnow provides a direct screen reader hint for Enter key behavior for expandable & interactive cells (#7448)