Repository navigation
fixing ordering of series in legend - #14113
Conversation
thomasneirynck
left a comment
There was a problem hiding this comment.
whoa, I like the small footprint of the fix a lot, and it makes sense to me to do that correction in tabify.
I only have one minor suggestion, not 100% sure if we should introduce a new side-effect in tabify.
There was a problem hiding this comment.
The reorder call creates a side-effect, fairly significant, and is only called from the tabify module. I'm not a 100% sure if this would have other impacts on the code, but it does break encapsulation somewhat.
I'd consider making the reorder() method return a clone of the buckets object, but with the keys ordered against the filters. So in tabify you'd have something like:
if (buckets.length) {
const reorderedBuckets = buckets.orderBucketsAccordingToFilterParams(agg.params);
//... and then just work with reorderedBuckets...
}
There was a problem hiding this comment.
Getting reorderedBuckets is undefined errors on this line because orderBucketsAccordingToFilterParams is still mutating this instead of returning a value.
c350eef to
d3fd759
Compare
|
@thomasneirynck i moved back to the original approach (changing the Buckets itself) i also added support for range and ip range aggregations |
There was a problem hiding this comment.
perhaps we can split the difference :)
Can we mutate in the constructor then, and keep this out of the flow of tabify.
//in tabify
const buckets = new Buckets(bucket[agg.id], agg.params);
and then in buckets
unction Buckets(aggResp, params) {
if (_.has(aggResp, 'buckets')) {
this.buckets = aggResp.buckets;
} else {
// Some Bucket Aggs only return a single bucket (like filter).
// In those instances, the aggResp is the content of the single bucket.
this.buckets = [aggResp];
}
this.objectMode = _.isPlainObject(this.buckets);
if (this.objectMode) {
this._keys = _.keys(this.buckets);
this.length = this._keys.length;
} else {
this.length = this.buckets.length;
}
if (this.length){
this.orderBucketsAccordingToParams(params);
}
}
|
@Bargs |
409644f to
ebd441f
Compare
|
Looks like they're passing now? |
|
yup, good for another look |
* fixing ordering of series produced on filters * updating based on review from Thomas * adding support for range and iprange aggregations * updating based on review from Thomas
* fixing ordering of series produced on filters * updating based on review from Thomas * adding support for range and iprange aggregations * updating based on review from Thomas
|
@ppisljar I know we discussed but can't recall where we ended up, did we end up backporting to 5.x or is this fixed in 6.0? |
|
PRs should have version labels on them |
* fixing ordering of series produced on filters * updating based on review from Thomas * adding support for range and iprange aggregations * updating based on review from Thomas
Summary: order of legend for series should be same as provided in the configuration.
Resolves #10543