Skip to content

Commit acbab6d

Browse files
cnathecnathe
committed
show range options for bar chart y-axis (but not linear / log)
1 parent 3f1f15f commit acbab6d

8 files changed

Lines changed: 47 additions & 50 deletions

‎packages/components/src/internal/components/chart/ChartBuilderModal.test.tsx‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,9 @@ describe('ChartBuilderModal', () => {
350350
expect(document.querySelector('input[name=y]').getAttribute('value')).toBe('field2');
351351
expect(document.querySelectorAll('.fa-gear')).toHaveLength(1); // gear icon for y-axis
352352
await userEvent.click(document.querySelector('.fa-gear'));
353-
expect(document.querySelectorAll('input')).toHaveLength(11);
353+
expect(document.querySelectorAll('input')).toHaveLength(13);
354+
expect(document.querySelector('input[value=automatic]').hasAttribute('checked')).toBe(true);
355+
expect(document.querySelector('input[value=manual]').hasAttribute('checked')).toBe(false);
354356
expect(document.querySelector('input[name=aggregate-method]').getAttribute('value')).toBe('SUM');
355357
expect(document.querySelectorAll('input[name=error-bar-method]')).toHaveLength(3);
356358
expect(document.querySelector('input[value=SD]').hasAttribute('checked')).toBe(false);
@@ -390,7 +392,9 @@ describe('ChartBuilderModal', () => {
390392
expect(document.querySelector('input[name=y]').getAttribute('value')).toBe('field2');
391393
expect(document.querySelectorAll('.fa-gear')).toHaveLength(1); // gear icon for y-axis
392394
await userEvent.click(document.querySelector('.fa-gear'));
393-
expect(document.querySelectorAll('input')).toHaveLength(11);
395+
expect(document.querySelectorAll('input')).toHaveLength(13);
396+
expect(document.querySelector('input[value=automatic]').hasAttribute('checked')).toBe(true);
397+
expect(document.querySelector('input[value=manual]').hasAttribute('checked')).toBe(false);
394398
expect(document.querySelector('input[name=aggregate-method]').getAttribute('value')).toBe('MEAN');
395399
expect(document.querySelectorAll('input[name=error-bar-method]')).toHaveLength(3);
396400
expect(document.querySelector('input[value=SD]').hasAttribute('checked')).toBe(false);

‎packages/components/src/internal/components/chart/ChartFieldAggregateOptions.test.tsx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ import React from 'react';
22
import { render } from '@testing-library/react';
33
import { userEvent } from '@testing-library/user-event';
44
import { ChartFieldAggregateOptions } from './ChartFieldAggregateOptions';
5-
import { BAR_CHART_AGGREGATE_NAME, BAR_CHART_ERROR_BAR_NAME } from "./constants";
5+
import { BAR_CHART_AGGREGATE_NAME, BAR_CHART_ERROR_BAR_NAME } from './constants';
6+
import { ChartTypeInfo } from './models';
67

78
const field = { name: 'testField', label: 'Test Label', required: false };
89
const fieldValues = {
@@ -16,10 +17,9 @@ function renderComponent(props = {}) {
1617
<ChartFieldAggregateOptions
1718
field={field}
1819
fieldValues={fieldValues}
19-
includeCount={true}
20-
includeNone={true}
2120
onErrorBarChange={jest.fn}
2221
onSelectFieldChange={jest.fn}
22+
selectedType={{ name: 'bar_chart' } as ChartTypeInfo}
2323
{...props}
2424
/>
2525
);

‎packages/components/src/internal/components/chart/ChartFieldAggregateOptions.tsx‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
import React, { FC, memo, useCallback, useMemo } from 'react';
22
import { OverlayTrigger } from '../../OverlayTrigger';
33
import { Popover } from '../../Popover';
4-
import { RadioGroupInput, RadioGroupOption } from '../forms/input/RadioGroupInput';
4+
import { RadioGroupInput } from '../forms/input/RadioGroupInput';
55

66
import { BAR_CHART_AGGREGATE_NAME, BAR_CHART_ERROR_BAR_NAME } from './constants';
7-
import { ChartFieldInfo } from './models';
7+
import { ChartFieldInfo, ChartTypeInfo } from './models';
88
import { LabelOverlay } from '../forms/LabelOverlay';
99
import { SelectInput, SelectInputOption } from '../forms/input/SelectInput';
1010

@@ -32,25 +32,18 @@ interface OwnProps {
3232
asOverlay?: boolean;
3333
field: ChartFieldInfo;
3434
fieldValues: Record<string, SelectInputOption>;
35-
includeCount: boolean;
36-
includeNone: boolean;
3735
onErrorBarChange: (name: string, value: string) => void;
3836
onSelectFieldChange: (name: string, value: string, selectedOption: SelectInputOption) => void;
37+
selectedType: ChartTypeInfo;
3938
}
4039

4140
export const ChartFieldAggregateOptions: FC<OwnProps> = memo(props => {
42-
const {
43-
field,
44-
fieldValues,
45-
onSelectFieldChange,
46-
onErrorBarChange,
47-
includeCount,
48-
includeNone,
49-
asOverlay = true,
50-
} = props;
41+
const { field, fieldValues, onSelectFieldChange, onErrorBarChange, asOverlay = true, selectedType } = props;
5142
const fieldValue = fieldValues?.[field.name];
5243
const aggregateValue = fieldValues?.[BAR_CHART_AGGREGATE_NAME]?.value;
5344
const errorBarValue = fieldValues?.[BAR_CHART_ERROR_BAR_NAME]?.value;
45+
const includeNone = selectedType.name === 'line_plot';
46+
const includeCount = selectedType.name === 'bar_chart';
5447
const defaultAggregateValue = useMemo(() => (includeNone ? '' : 'SUM'), [includeNone]);
5548
const errorBarRadioEnabled = useMemo(() => aggregateValue === 'MEAN', [aggregateValue]);
5649

‎packages/components/src/internal/components/chart/ChartFieldOption.tsx‎

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,6 @@ export const ChartFieldOption: FC<OwnProps> = memo(props => {
7070
);
7171
const showRangeScaleOptions = isNumericType && shouldShowRangeScaleOptions(field, selectedType);
7272
const showAggregateOptions = isNumericType && shouldShowAggregateOptions(field, selectedType);
73-
const isBar = selectedType.name === 'bar_chart';
74-
const isLine = selectedType.name === 'line_plot';
7573

7674
// Issue 52050: use fieldKey for special characters
7775
const selectInputValue = useMemo(() => fieldValue?.data.fieldKey ?? fieldValue?.value, [fieldValue]);
@@ -108,30 +106,20 @@ export const ChartFieldOption: FC<OwnProps> = memo(props => {
108106
onScaleChange={onScaleChange}
109107
scale={scale}
110108
setScale={setScale}
109+
showScaleTrans={selectedType.name !== 'bar_chart'}
111110
>
112-
{isLine && showAggregateOptions && (
111+
{showAggregateOptions && (
113112
<ChartFieldAggregateOptions
114113
asOverlay={false}
115114
field={field}
116115
fieldValues={fieldValues}
117-
includeCount={isBar}
118-
includeNone={isLine}
119116
onErrorBarChange={onErrorBarChange}
120117
onSelectFieldChange={onSelectFieldChange}
118+
selectedType={selectedType}
121119
/>
122120
)}
123121
</ChartFieldRangeScaleOptions>
124122
)}
125-
{isBar && showAggregateOptions && (
126-
<ChartFieldAggregateOptions
127-
field={field}
128-
fieldValues={fieldValues}
129-
includeCount={isBar}
130-
includeNone={isLine}
131-
onErrorBarChange={onErrorBarChange}
132-
onSelectFieldChange={onSelectFieldChange}
133-
/>
134-
)}
135123
</div>
136124
</div>
137125
);

‎packages/components/src/internal/components/chart/ChartFieldRangeScaleOptions.test.tsx‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,19 @@ import React from 'react';
22
import { render } from '@testing-library/react';
33
import { userEvent } from '@testing-library/user-event';
44
import { ChartFieldRangeScaleOptions } from './ChartFieldRangeScaleOptions';
5+
import { ScaleType } from './models';
56

67
const field = { name: 'testField', label: 'Test Label', required: false };
78

8-
function renderComponent(scale = {}) {
9+
function renderComponent(scale = {} as ScaleType) {
910
return render(
10-
<ChartFieldRangeScaleOptions field={field} onScaleChange={jest.fn} scale={scale} setScale={jest.fn}>
11+
<ChartFieldRangeScaleOptions
12+
field={field}
13+
onScaleChange={jest.fn}
14+
scale={scale}
15+
setScale={jest.fn}
16+
showScaleTrans
17+
>
1118
<div className="child-content">Children Content</div>
1219
</ChartFieldRangeScaleOptions>
1320
);
@@ -62,12 +69,12 @@ describe('ChartFieldRangeScaleOptions', () => {
6269
});
6370

6471
test('does not show invalid range warning when min is undefined', async () => {
65-
renderComponent({ type: 'manual', min: undefined, max: 10 });
72+
renderComponent({ type: 'manual', min: undefined, max: 10 } as ScaleType);
6673
await userEvent.click(document.querySelector('.fa-gear'));
6774
expect(document.querySelectorAll('.text-danger')).toHaveLength(0);
6875
});
6976
test('does not show invalid range warning when max is undefined', async () => {
70-
renderComponent({ type: 'manual', min: 5, max: undefined });
77+
renderComponent({ type: 'manual', min: 5, max: undefined } as ScaleType);
7178
await userEvent.click(document.querySelector('.fa-gear'));
7279
expect(document.querySelectorAll('.text-danger')).toHaveLength(0);
7380
});

‎packages/components/src/internal/components/chart/ChartFieldRangeScaleOptions.tsx‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,12 @@ interface OwnProps extends PropsWithChildren {
2020
onScaleChange: (field: string, key: string, value: number | string, reset?: boolean) => void;
2121
scale: ScaleType;
2222
setScale: (scale: ScaleType) => void;
23+
showScaleTrans: boolean;
2324
}
2425

2526
export const ChartFieldRangeScaleOptions: FC<OwnProps> = memo(props => {
26-
const { field, scale, setScale, onScaleChange, children } = props;
27+
const { field, scale, setScale, onScaleChange, showScaleTrans, children } = props;
28+
const placement = useMemo(() => (!showScaleTrans && children ? 'left' : 'bottom'), [showScaleTrans, children]);
2729

2830
const scaleTransOptions = useMemo(() => {
2931
return SCALE_TRANS_TYPES.map(option => ({ ...option, selected: scale.trans === option.value }));
@@ -89,17 +91,19 @@ export const ChartFieldRangeScaleOptions: FC<OwnProps> = memo(props => {
8991
<div className="field-option-icon">
9092
<OverlayTrigger
9193
overlay={
92-
<Popover id="chart-field-option-popover" placement="bottom">
94+
<Popover id="chart-field-option-popover" placement={placement}>
9395
{children}
94-
<div className="field-option-radio-group">
95-
<label>Scale</label>
96-
<RadioGroupInput
97-
formsy={false}
98-
name="scaleTrans"
99-
onValueChange={onScaleTransChange}
100-
options={scaleTransOptions}
101-
/>
102-
</div>
96+
{showScaleTrans && (
97+
<div className="field-option-radio-group">
98+
<label>Scale</label>
99+
<RadioGroupInput
100+
formsy={false}
101+
name="scaleTrans"
102+
onValueChange={onScaleTransChange}
103+
options={scaleTransOptions}
104+
/>
105+
</div>
106+
)}
103107
<div className="field-option-radio-group">
104108
<label>Range</label>
105109
<RadioGroupInput

‎packages/components/src/internal/components/chart/utils.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ const yField = { name: 'y' } as ChartFieldInfo;
328328
describe('shouldShowRangeScaleOptions', () => {
329329
test('based on chart type', () => {
330330
expect(shouldShowRangeScaleOptions(xField, BAR_CHART_TYPE)).toBe(false);
331-
expect(shouldShowRangeScaleOptions(yField, BAR_CHART_TYPE)).toBe(false);
331+
expect(shouldShowRangeScaleOptions(yField, BAR_CHART_TYPE)).toBe(true);
332332
expect(shouldShowRangeScaleOptions(xField, BOX_PLOT_TYPE)).toBe(false);
333333
expect(shouldShowRangeScaleOptions(yField, BOX_PLOT_TYPE)).toBe(true);
334334
expect(shouldShowRangeScaleOptions(xField, SCATTER_PLOT_TYPE)).toBe(true);

‎packages/components/src/internal/components/chart/utils.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,9 @@ export const shouldShowRangeScaleOptions = (field: ChartFieldInfo, selectedType:
8080
const isScatter = selectedType.name === 'scatter_plot';
8181
const isLine = selectedType.name === 'line_plot';
8282
const isBox = selectedType.name === 'box_plot';
83+
const isBar = selectedType.name === 'bar_chart';
8384
const showForX = field.name === 'x' && (isScatter || isLine);
84-
const showForY = field.name === 'y' && (isScatter || isLine || isBox);
85+
const showForY = field.name === 'y' && (isScatter || isLine || isBox || isBar);
8586
return showForX || showForY;
8687
};
8788

0 commit comments

Comments
 (0)