EnxDev commented on code in PR #44556:
URL: https://github.com/apache/superset/pull/44556#discussion_r4087585934
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx:
##########
@@ -246,9 +246,10 @@ function createAxisControl(axis: 'x' | 'y'):
ControlSetRow[] {
label: t('Logarithmic axis'),
renderTrigger: true,
default: logAxis,
- description: t('Logarithmic axis'),
+ description: t('Logarithmic axis is not available for stacked bars'),
Review Comment:
The checkbox is hidden whenever bars are stacked, so outside the saved
stack+log case this tooltip is only seen when log is available, and there it
reads as if it isn't.
I'd keep the original description here and let the `stack` warning carry the
explanation.
```suggestion
description: t('Logarithmic axis'),
```
(That also drops the new msgid from `messages.pot`.)
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/controlPanel.test.ts:
##########
@@ -330,3 +334,56 @@ test('x_axis_time_format should be hidden for numeric
columns', () => {
false,
);
});
+
+const logControls = config.controlPanelSections
+ .flatMap(section => (section && section.controlSetRows) || [])
+ .flat()
+ .filter(isCustomControlItem)
+ .filter(control => control.name === 'logAxis');
+
+test.each([OrientationType.Vertical, OrientationType.Horizontal])(
+ 'prevents logarithmic selection while %s bars are stacked, and restores it
when cleared',
+ orientation => {
+ expect(logControls).toHaveLength(2);
+ const state = {
+ controls: {
+ orientation: { value: orientation },
+ stack: { value: StackControlsValue.Stack },
+ logAxis: { value: false },
+ },
+ } as unknown as ControlPanelsContainerProps;
+ expect(
+ logControls.map(control => control.config.visibility!(state, {})),
+ ).toEqual([false, false]);
+ state.controls.stack.value = null;
+ expect(
+ logControls.filter(control => control.config.visibility!(state, {})),
+ ).toHaveLength(1);
+ state.controls.stack.value = StackControlsValue.Stack;
+ state.controls.logAxis.value = true;
+ // A saved chart with both settings must retain a route to turn log mode
off.
+ expect(
+ logControls.filter(control => control.config.visibility!(state, {})),
+ ).toHaveLength(1);
+ },
+);
+
+test.each<[boolean, StackControlsValue | null, boolean]>([
+ [true, null, true],
+ [false, null, false],
+ [true, StackControlsValue.Stack, false],
+ [false, StackControlsValue.Stack, false],
+])(
+ 'stack control with log=%s and stack=%s is disabled=%s',
+ (logAxis, stack, disabled) => {
+ const state = {
+ controls: { logAxis: { value: logAxis }, stack: { value: stack } },
+ } as unknown as ControlPanelState;
+ expect(
+ config.controlOverrides!.stack!.mapStateToProps!(state, {
+ type: 'SelectControl',
+ value: stack,
+ }),
+ ).toMatchObject({ disabled });
Review Comment:
Nit, take it or leave it. This only checks `disabled`, but the warning is
the one thing telling the user why Stacked Style is greyed out, and nothing
fails if it goes missing.
```suggestion
).toMatchObject({
disabled,
warning: logAxis ? expect.any(String) : null,
});
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
