-
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
[AIOps] Explain Log Rate Spikes: create shareable component containing only analysis #158629
[AIOps] Explain Log Rate Spikes: create shareable component containing only analysis #158629
Conversation
Pinging @elastic/ml-ui (:ml) |
cc @benakansara |
16bb638
to
38c7f01
Compare
)} | ||
<EuiHorizontalRule /> | ||
</EuiResizablePanel> | ||
{/* <EuiResizableButton /> */} |
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 for a follow-up allowing the two panels to be resized?
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.
Yes - just too much code churn to add in here 👍
} | ||
|
||
return ( | ||
<EuiResizableContainer style={{ height: '400px' }} direction="vertical"> |
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.
Do you have to specify a height here? Can it have a responsive layout so that it adapts to the available screen height?
<EuiResizablePanel | ||
paddingSize="s" | ||
mode={'main'} | ||
initialSize={80} |
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.
Shouldn't the two values for initialSize
add up to 100%?
<> | ||
<EuiResizablePanel | ||
mode={'collapsible'} | ||
initialSize={60} |
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.
This panel is always getting a scrollbar for me. Does the initialSize
or minSize
need to be larger?
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.
LGTM, pending fixed functional tests and fixing the scrollbar regression.
const resultsGroupedOffId = 'aiopsExplainLogRateSpikesGroupingOff'; | ||
const resultsGroupedOnId = 'aiopsExplainLogRateSpikesGroupingOn'; |
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: Could be RESULTS_GROUPING_ID_OFF/ON
as a const
and to match the id a bit more.
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.
LGTM. Re-enable the resizable panel layout in a follow-up PR.
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
This PR exposes the
ExplainLogRateSpikesContent
shared component so that it can be used independently of the search bar/datepickerExplainLogRateSpikesPage
component now uses theExplainLogRateSpikesContent
componentuseData
hook has been simplified - the set up for the search query has been extracted into a separate hookThis is the first step for the Observability Alert Details Page Integration.
Style edits:
The component now uses EUI's ResizeableContainer to allow the main histogram to be sticky.
Also adds some style updates from #156605
Checklist
Delete any items that are not applicable to this PR.