-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[RUM Dashboard] Rum usability improvement #76024
[RUM Dashboard] Rum usability improvement #76024
Conversation
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.
Left some comments. The i18n definitely needs added, but use your discretion for the rest of my comments.
{ | ||
name: 'Browser', | ||
name: '- No breakdown -', |
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.
All of these name
properties should have i18n translations.
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.
done
); | ||
|
||
const onBreakdownChange = (values: BreakdownItem[]) => { | ||
setBreakdowns(values); | ||
const onBreakdownChange = (value: BreakdownItem | 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.
Do you need to define this function? Couldn't you just pass onBreakdownChange={setBreakdown}
into the BreakdownFilter
?
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.
done
breakdown: { | ||
terms: { | ||
field: breakdownItem.fieldName, | ||
size: 9, |
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 9
?
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 was mentioned in the issue that we need to show top 9 by volume #69712
{ | ||
name: 'Browser', | ||
name: '- No breakdown -', |
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 don't think the -
adds anything. No breakdown
(with i18n) would be fine.
x-pack/plugins/apm/public/components/app/RumDashboard/Breakdowns/BreakdownFilter.tsx
Show resolved
Hide resolved
Pinging @elastic/apm-ui (Team:apm) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
Fixes #74835
Used a select component for breakdown selector