-
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
[Viz] legend duplicates percentile options when chart has both left & right Y axes #113073
[Viz] legend duplicates percentile options when chart has both left & right Y axes #113073
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.
Excepting the suggestion, the code LGTM. Tested locally, everything works as expected.
@@ -77,3 +82,11 @@ export const getSplitSeriesAccessorFnMap = ( | |||
|
|||
return m; | |||
}; | |||
|
|||
// For percentile aggregation id is coming in the form `%d.%d`, where first `%d` is `id` and the second - `percents` |
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: The mistake in this comment is my fault. I'd suggest changing %d.%d
to %s.%d
. The final comment would be:
- // For percentile, the aggregation id is coming in the form
%s.%d
, where%s
isagg_id
and%d
-percents
.
Pinging @elastic/kibana-vis-editors (Team:VisEditors) |
@elasticmachine merge upstream |
import { Aspect } from '../types'; | ||
import type { Aspect } from '../types'; | ||
|
||
interface Dimension { |
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 type here confuses me because we already have defined a type Dimension
in the types folder. Is it different from this type? If yes, it should have a different name. If no, the already existing type should be used
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! I tested it Chrome and works fine :D
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.
Changes LGTM )
… right Y axes (elastic#113073) * [Viz] legend duplicates percentile options when chart has both left & right Y axes * Update comment for isPercentileIdEqualToSeriesId * Remove Dimension interface * Replace partial aspect with whole aspect value Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
… right Y axes (#113073) (#113169) * [Viz] legend duplicates percentile options when chart has both left & right Y axes * Update comment for isPercentileIdEqualToSeriesId * Remove Dimension interface * Replace partial aspect with whole aspect value Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Closes #112388
Partial changes from #111378
Summary
The reason for incorrect displaying of chart and its legend was ignoring the case for percentile column id. As it contains percent value after the point, we should take into account only id part before it, while filtering
yAspects
.Checklist
For maintainers