-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[RAC][Analyzer] Make analyzer full screen mode compatible with EuiDataGrid's #110723
[RAC][Analyzer] Make analyzer full screen mode compatible with EuiDataGrid's #110723
Conversation
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
@@ -147,6 +147,9 @@ export class Plugin implements IPlugin<PluginSetup, PluginStart, SetupPlugins, S | |||
const [coreStart, startPlugins] = await core.getStartServices(); | |||
const subPlugins = await this.startSubPlugins(this.storage, coreStart, startPlugins); | |||
const { renderApp } = await this.lazyApplicationDependencies(); | |||
if (startPlugins.timelines && this._store) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not need that anymore because of this commit da68c5d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still working as expected in timeline fly-out?
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Alerting API Integration Tests.x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/find·ts.alerting api integration security and spaces enabled Alerts alerts find "after each" hook for "should handle find alert request with fields appropriately"Standard Out
Stack Trace
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
const onMutation = useCallback(() => { | ||
const isExpanded = document.querySelector('.euiDataGrid--fullScreen') !== null; | ||
setDataGridExpanded(isExpanded); | ||
}, [setDataGridExpanded]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you should be able to remove this setter as a dependency because it is coming from useState
@@ -734,7 +734,6 @@ export const BodyComponent = React.memo<StatefulBodyProps>( | |||
); | |||
|
|||
const height = useDataGridHeightHack(pageSize, data.length); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit -> we can avoid a change file here
@@ -29,6 +29,25 @@ export const resetScroll = () => { | |||
}, 0); | |||
}; | |||
|
|||
export const setDataGridFullScreen = (isExpanded: boolean) => { | |||
const clickEvent = new KeyboardEvent('click', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please put a // TODO
with a link to the Eui issue so we remember to come back and refactor once a method is available. @chandlerprall could you please help us find an eui issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gutterSize="none" | ||
> | ||
<ScrollableFlexItem grow={1}> | ||
{nonDeletedEvents.length === 0 && loading === false ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not still using the same logic than before with totalCountMinusDeleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry bad merge will fix
} | ||
/> | ||
)} | ||
{totalCountMinusDeleted > 0 && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we did that to avoid the EuiDatagrid to render with no data because we were getting a weird UX with the EuiDatagrid when we did not have data
)} | ||
</UpdatedFlexGroup> | ||
|
||
{!graphEventId && graphOverlay == null && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you do not want this condition anymore???
cancelable: true, | ||
}); | ||
const dataGridIsFullScreen = document.querySelector('.euiDataGrid--fullScreen') !== null; | ||
const dataGridFullScreenButtonNew = document.querySelector( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my hacky sense love it, however my gray hair are telling me to not go for it and just ask EUI to create a callback on the full screen action. I really appreciate the ingenuity here.
I am sorry if I disappoint you but a safer bet it is to go with @stephmilovic 's PR to just disable the full screen on the analyzer for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulewing are you ok with that?
closing this for an infinitely simpler way of doing the same thing 😆 😬 |
Summary
This pr enables the analyzer to work with EuiDataGrid full screen mode, by making use of EuiMutationObserver, a query selector to see when the data grid is actually expanded, and some artificial click events to open/close EuiDataGrid's full screen mode as appropriate when a user is using resolver.
Checklist
Delete any items that are not applicable to this PR.