Skip to content
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

fix(interactions): change allowBrushingLastHistogramBin to true #1396

Merged
2 changes: 1 addition & 1 deletion packages/charts/api/charts.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1850,7 +1850,7 @@ export const Settings: React_2.FunctionComponent<SettingsSpecProps>;

// @public
export interface SettingsSpec extends Spec, LegendSpec {
allowBrushingLastHistogramBucket?: boolean;
allowBrushingLastHistogramBin?: boolean;
// (undocumented)
animateData: boolean;
ariaDescribedBy?: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { Selector } from 'reselect';

import { ChartType } from '../../..';
import { Scale } from '../../../../scales';
import { GroupBrushExtent, XYBrushEvent } from '../../../../specs';
import { GroupBrushExtent, SeriesSpecs, XYBrushEvent } from '../../../../specs';
import { BrushAxis } from '../../../../specs/constants';
import { DragState, GlobalChartState } from '../../../../state/chart_state';
import { createCustomCachedSelector } from '../../../../state/create_selector';
Expand All @@ -19,11 +19,13 @@ import { clamp, Rotation } from '../../../../utils/common';
import { Dimensions } from '../../../../utils/dimensions';
import { hasDragged, DragCheckProps } from '../../../../utils/events';
import { GroupId } from '../../../../utils/ids';
import { isHistogramEnabled } from '../../domains/y_domain';
import { isVerticalRotation } from '../utils/common';
import { computeChartDimensionsSelector } from './compute_chart_dimensions';
import { computeSmallMultipleScalesSelector, SmallMultipleScales } from './compute_small_multiple_scales';
import { getPlotAreaRestrictedPoint, getPointsConstraintToSinglePanel, PanelPoints } from './get_brush_area';
import { getComputedScalesSelector } from './get_computed_scales';
import { getSeriesSpecsSelector } from './get_specs';
import { isBrushAvailableSelector } from './is_brush_available';
import { isHistogramModeEnabledSelector } from './is_histogram_mode_enabled';

Expand Down Expand Up @@ -53,21 +55,16 @@ export function createOnBrushEndCaller(): (state: GlobalChartState) => void {
computeChartDimensionsSelector,
isHistogramModeEnabledSelector,
computeSmallMultipleScalesSelector,
getSeriesSpecsSelector,
],
(
lastDrag,
{
onBrushEnd,
rotation,
brushAxis,
minBrushDelta,
roundHistogramBrushValues,
allowBrushingLastHistogramBucket,
},
{ onBrushEnd, rotation, brushAxis, minBrushDelta, roundHistogramBrushValues, allowBrushingLastHistogramBin },
computedScales,
{ chartDimensions },
histogramMode,
smallMultipleScales,
seriesSpec,
): void => {
const nextProps = {
lastDrag,
Expand All @@ -85,9 +82,10 @@ export function createOnBrushEndCaller(): (state: GlobalChartState) => void {
histogramMode,
xScale as Scale<number>,
smallMultipleScales,
allowBrushingLastHistogramBin,
seriesSpec,
minBrushDelta,
roundHistogramBrushValues,
allowBrushingLastHistogramBucket,
);
}
if (brushAxis === BrushAxis.Y || brushAxis === BrushAxis.Both) {
Expand Down Expand Up @@ -136,9 +134,10 @@ function getXBrushExtent(
histogramMode: boolean,
xScale: Scale<number>,
smallMultipleScales: SmallMultipleScales,
allowBrushingLastHistogramBin: boolean = true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great thanks! 1f5186a

seriesSpecs: SeriesSpecs,
minBrushDelta?: number,
roundHistogramBrushValues?: boolean,
allowBrushingLastHistogramBucket?: boolean,
): [number, number] | undefined {
const isXHorizontal = !isVerticalRotation(rotation);
// scale screen coordinates down to panel scale
Expand All @@ -153,15 +152,16 @@ function getXBrushExtent(
// if 0 size brush, avoid computing the value
return;
}

const offset = histogramMode ? 0 : -(xScale.bandwidth + xScale.bandwidthPadding) / 2;
const invertValue = roundHistogramBrushValues
? (value: number) => xScale.invertWithStep(value, xScale.domain).value
: (value: number) => xScale.invert(value);
const histogramEnabled = isHistogramEnabled(seriesSpecs);
const invertValue =
histogramEnabled && roundHistogramBrushValues
? (value: number) => xScale.invertWithStep(value, xScale.domain).value
: (value: number) => xScale.invert(value);
const minPosScaled = invertValue(minPos + offset);
const maxPosScaled = invertValue(maxPos + offset);

const maxDomainValue = xScale.domain[1] + (allowBrushingLastHistogramBucket ? xScale.minInterval : 0);
const maxDomainValue =
xScale.domain[1] + (histogramEnabled && allowBrushingLastHistogramBin ? xScale.minInterval : 0);

const minValue = clamp(minPosScaled, xScale.domain[0], maxPosScaled);
const maxValue = clamp(minPosScaled, maxPosScaled, maxDomainValue);
Expand Down
6 changes: 3 additions & 3 deletions packages/charts/src/specs/settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -582,16 +582,16 @@ export interface SettingsSpec extends Spec, LegendSpec {
roundHistogramBrushValues?: boolean;
/**
* Boolean to allow brushing on last bucket even when outside domain or limit to end of domain.
*
* Should apply only for histogram charts
* e.g.
* A brush selection range of [1.23, 3.6] with a domain of [1, 2, 3]
*
* - when true returns [1.23, 3.6]
* - when false returns [1.23, 3]
*
* @defaultValue false
* @defaultValue true
*/
allowBrushingLastHistogramBucket?: boolean;
allowBrushingLastHistogramBin?: boolean;
/**
* Orders ordinal x values
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const Example = () => (
onBrushEnd={action('onBrushEnd')}
rotation={getChartRotationKnob()}
roundHistogramBrushValues={boolean('roundHistogramBrushValues', false)}
allowBrushingLastHistogramBucket={boolean('allowBrushingLastHistogramBucket', false)}
allowBrushingLastHistogramBin={boolean('allowBrushingLastHistogramBin', false)}
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
/>
<Axis id="bottom" position={Position.Bottom} title="bottom" showOverlappingTicks />
<Axis id="left" title="left" position={Position.Left} tickFormat={(d) => Number(d).toFixed(2)} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export const Example = () => {
onElementClick={action('onElementClick')}
rotation={getChartRotationKnob()}
roundHistogramBrushValues={boolean('roundHistogramBrushValues', false)}
allowBrushingLastHistogramBucket={boolean('allowBrushingLastHistogramBucket', false)}
allowBrushingLastHistogramBin={boolean('allowBrushingLastHistogramBin', false)}
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
/>
<Axis
id="bottom"
Expand Down