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
4 changes: 2 additions & 2 deletions packages/charts/api/charts.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,7 @@ export const DEFAULT_TOOLTIP_SNAP = true;
export const DEFAULT_TOOLTIP_TYPE: "vertical";

// @public (undocumented)
export type DefaultSettingsProps = 'id' | 'chartType' | 'specType' | 'rendering' | 'rotation' | 'resizeDebounce' | 'pointerUpdateDebounce' | 'pointerUpdateTrigger' | 'animateData' | 'debug' | 'tooltip' | 'theme' | 'brushAxis' | 'minBrushDelta' | 'externalPointerEvents' | 'showLegend' | 'showLegendExtra' | 'legendPosition' | 'legendMaxDepth' | 'ariaUseDefaultSummary' | 'ariaLabelHeadingLevel' | 'ariaTableCaption';
export type DefaultSettingsProps = 'id' | 'chartType' | 'specType' | 'rendering' | 'rotation' | 'resizeDebounce' | 'pointerUpdateDebounce' | 'pointerUpdateTrigger' | 'animateData' | 'debug' | 'tooltip' | 'theme' | 'brushAxis' | 'minBrushDelta' | 'externalPointerEvents' | 'showLegend' | 'showLegendExtra' | 'legendPosition' | 'legendMaxDepth' | 'ariaUseDefaultSummary' | 'ariaLabelHeadingLevel' | 'ariaTableCaption' | 'allowBrushingLastHistogramBin';

// @public (undocumented)
export const DEPTH_KEY = "depth";
Expand Down 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,
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
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ exports[`Chart should render the legend name test 1`] = `
<Connect(SpecsParserComponent)>
<SpecsParserComponent specParsed={[Function (anonymous)]} specUnmounted={[Function (anonymous)]}>
<Connect(SpecInstance) debug={true} rendering=\\"svg\\" showLegend={true}>
<SpecInstance debug={true} rendering=\\"svg\\" showLegend={true} upsertSpec={[Function (anonymous)]} removeSpec={[Function (anonymous)]} id=\\"__global__settings___\\" chartType=\\"global\\" specType=\\"settings\\" rotation={0} animateData={true} resizeDebounce={10} tooltip={{...}} pointerUpdateTrigger=\\"x\\" externalPointerEvents={{...}} baseTheme={{...}} brushAxis=\\"x\\" minBrushDelta={2} ariaUseDefaultSummary={true} ariaLabelHeadingLevel=\\"p\\" showLegendExtra={false} legendMaxDepth={Infinity} legendPosition=\\"right\\" />
<SpecInstance debug={true} rendering=\\"svg\\" showLegend={true} upsertSpec={[Function (anonymous)]} removeSpec={[Function (anonymous)]} id=\\"__global__settings___\\" chartType=\\"global\\" specType=\\"settings\\" rotation={0} animateData={true} resizeDebounce={10} tooltip={{...}} pointerUpdateTrigger=\\"x\\" externalPointerEvents={{...}} baseTheme={{...}} brushAxis=\\"x\\" minBrushDelta={2} ariaUseDefaultSummary={true} ariaLabelHeadingLevel=\\"p\\" allowBrushingLastHistogramBin={true} showLegendExtra={false} legendMaxDepth={Infinity} legendPosition=\\"right\\" />
</Connect(SpecInstance)>
<Connect(SpecInstance) id=\\"test\\" data={{...}}>
<SpecInstance id=\\"test\\" data={{...}} upsertSpec={[Function (anonymous)]} removeSpec={[Function (anonymous)]} chartType=\\"xy_axis\\" specType=\\"series\\" seriesType=\\"bar\\" groupId=\\"__global__\\" xScaleType=\\"ordinal\\" yScaleType=\\"linear\\" xAccessor=\\"x\\" yAccessors={{...}} hideInLegend={false} enableHistogramMode={false} />
Expand Down
1 change: 1 addition & 0 deletions packages/charts/src/specs/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,5 +181,6 @@ export const DEFAULT_SETTINGS_SPEC: SettingsSpec = {
minBrushDelta: 2,
ariaUseDefaultSummary: true,
ariaLabelHeadingLevel: 'p',
allowBrushingLastHistogramBin: true,
...DEFAULT_LEGEND_CONFIG,
};
9 changes: 5 additions & 4 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice removal of an optional here 😍 Also, the Bucket -> Bin change 👍

/**
* Orders ordinal x values
*/
Expand Down Expand Up @@ -704,7 +704,8 @@ export type DefaultSettingsProps =
| 'legendMaxDepth'
| 'ariaUseDefaultSummary'
| 'ariaLabelHeadingLevel'
| 'ariaTableCaption';
| 'ariaTableCaption'
| 'allowBrushingLastHistogramBin';

/** @public */
export type SettingsSpecProps = Partial<Omit<SettingsSpec, 'chartType' | 'specType' | 'id'>>;
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