fix(interactions): change allowBrushingLastHistogramBin to true #1396
Conversation
rshen91
changed the title
@markov00 -- What are your thoughts about removing this setting altogether? |
There was a problem hiding this comment.
Looking at the changes in the test looks like that we aren't properly using the allowBrushingLastHistogramBucket with the required constraint that the series needs to be an Histogram (histogramEnabled=true). Instead it looks like we just forget about that and we use directly allowBrushingLastHistogramBucket in the calculation like
const maxDomainValue = xScale.domain[1] + (allowBrushingLastHistogramBucket ? xScale.minInterval : 0);
This create the issue that is visible in the test: in a simple bar chart, with the current default enabled allowBrushingLastHistogramBucket we are changing the behavior of the brush. The scope of the allowBrushingLastHistogramBucket should only apply to histograms.
I've noticed the same wrong logic applied to the roundHistogramBrushValues
we are rounding brush values also to non histogram based charts.
@nickofthyme it looks like that you added those properties, do you remember the reason why these logics are not strictly applied to histogram only charts?
@markov00 I think you are correct, that should only apply to histogram mode. |
rshen91
changed the title
| @@ -136,9 +134,10 @@ function getXBrushExtent( | |||
| histogramMode: boolean, | |||
| xScale: Scale<number>, | |||
| smallMultipleScales: SmallMultipleScales, | |||
| allowBrushingLastHistogramBin: boolean = true, | |||
There was a problem hiding this comment.
Nit: allowBrushingLastHistogramBin is a mandatory argument to getXBrushExtent which we can tell from the presence of a parameter that's to the right of it, seriesSpecs, which is mandatory (no question mark, no default). So wherever getXBrushExtent is used, we already pass something for allowBrushingLastHistogramBin otherwise the type checkers would shout. So the default value = true can be removed, assuming we are not passing the function undefined for this param (this is the only case the true default value would be in effect). So it's worth revisiting the caller places, and pass true instead of undefined if we currently pass undefined anywhere.
This is a very small thing that supports the larger theme of type simplicity: it's simpler to have boolean than boolean | undefined. Union types like this, and optional parameters should preferably be avoided, unless it leads to convoluted code
There was a problem hiding this comment.
This is great thanks! 1f5186a
…ingLastHistogramBucket
storybook/stories/interactions/10a_brush_selection_bar_hist.story.tsx
Outdated
Show resolved
Hide resolved
|
@rshen91 make sure you note the rename here as a breaking change. |
nickofthyme
added
:interactions
| */ | ||
| allowBrushingLastHistogramBucket?: boolean; | ||
| allowBrushingLastHistogramBin: boolean; |
There was a problem hiding this comment.
Nice removal of an optional here 😍 Also, the Bucket -> Bin change 👍
# [38.0.0](v37.0.0...v38.0.0) (2021-10-15) ### Bug Fixes * **deps:** update dependency @elastic/eui to v39 ([#1422](#1422)) ([2ee97aa](2ee97aa)) * **goal:** reduce whitespace for circular charts ([#1413](#1413)) ([6517523](6517523)) * **interactions:** change allowBrushingLastHistogramBin to true ([#1396](#1396)) ([9fa9783](9fa9783)) * **xy:** remove wrongly represented null/missing values in tooltip ([#1415](#1415)) ([e5963a3](e5963a3)), closes [#1414](#1414) ### Code Refactoring * scales ([#1410](#1410)) ([a53a2ba](a53a2ba)) ### Features * **scales:** add `LinearBinary` scale type ([#1389](#1389)) ([9f2e427](9f2e427)) * **xy:** adaptive tick raster ([#1420](#1420)) ([200577b](200577b)) * **xy:** apply the data value formatter to data values over bars ([#1419](#1419)) ([e673fc7](e673fc7)) ### BREAKING CHANGES * **interactions:** allowBrushingLastHistogramBucket renamed to allowBrushingLastHistogramBin on the Settings component defaults true and is only applied for histogram type charts * LogScaleOptions.logBase` is now a `number` instead of the object enum `LogBase`. Some edge case data or configuration _might_, with a deemed low likelihood, lead to a situation where the earlier version would have silently not rendered a bar, line or point, while the new code doesn't `catch`, therefore throw an exception (see the last item). General risk of regressions due to the quantity of code changes (altogether 3.5k)
|
🎉 This PR is included in version 38.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
This PR changes the default boolean value of allowBrushingLastHIstogramBucket to true instead of false. This setting change more accurately shows the correct value when brushing over the last bucket in a histogram
BREAKING CHANGE
allowBrushingLastHistogramBucket renamed to allowBrushingLastHistogramBin on the Settings component defaults true and is only applied for histogram type charts.
Details
This setting wasn't removed and users are still able to set this to false if they would prefer. The naming of this setting was changed to bin instead of bucket after team discussion.
Issues
Closes #1371
Checklist
:interactions,:axis)closes #123,fixes #123)