rusackas commented on code in PR #34742:
URL: https://github.com/apache/superset/pull/34742#discussion_r3330211249
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -237,6 +237,40 @@ export default function RangeFilterPlugin(props:
PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
+ // Use a step that gives approximately 100 steps across the range
+ return Math.pow(10, -Math.min(decimalPlaces + 1, 6)) || 0.001;
Review Comment:
Declining — the `|| 0.0001` fallback is reachable. After `Math.round(step /
magnitude) * magnitude`, `step` can be exactly 0 when `step/magnitude` is small
enough to round down (e.g., range of 0.00000001 gives `step/magnitude ≈ 0.01`,
which rounds to 0). The guard prevents returning 0 to the slider, which would
break it.
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -237,6 +237,40 @@ export default function RangeFilterPlugin(props:
PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
+ // Use a step that gives approximately 100 steps across the range
+ return Math.pow(10, -Math.min(decimalPlaces + 1, 6)) || 0.001;
+ }
+ return 0.01;
+ }
+
+ // For larger ranges, calculate step to give approximately 100-1000 steps
+ const idealSteps = 100;
+ let step = range / idealSteps;
+
+ // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
+ const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
+ step = Math.ceil(step / magnitude) * magnitude;
Review Comment:
Already addressed — the current code uses `Math.round` (not `Math.ceil`).
Agreed that `Math.round` is the right choice.
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.test.tsx:
##########
@@ -322,4 +322,127 @@ describe('RangeFilterPlugin', () => {
expect(sliders.length).toBeGreaterThan(0);
});
});
+
+ describe('Decimal value handling', () => {
+ it('should handle decimal ranges correctly (0.03 to 1.08)', () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.03, max: 1.08 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.5, 0.8] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs).toHaveLength(2);
+ expect(inputs[0]).toHaveValue('0.5');
+ expect(inputs[1]).toHaveValue('0.8');
+
+ // Verify the slider exists and can handle decimal values
+ const sliders = screen.getAllByRole('slider');
+ expect(sliders.length).toBeGreaterThan(0);
+ });
+
+ it('should calculate appropriate step size for small decimal ranges', ()
=> {
+ const smallRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.001, max: 0.01 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.005, 0.008] },
+ };
+ getWrapper(smallRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('0.005');
+ expect(inputs[1]).toHaveValue('0.008');
+ });
+
+ it('should handle very large ranges with appropriate step size', () => {
+ const largeRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0, max: 1000000 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [100000, 500000] },
+ };
+ getWrapper(largeRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('100000');
+ expect(inputs[1]).toHaveValue('500000');
+ });
+
+ it('should handle negative decimal ranges', () => {
+ const negativeDecimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: -1.5, max: 2.5 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [-0.5, 1.5] },
+ };
+ getWrapper(negativeDecimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('-0.5');
+ expect(inputs[1]).toHaveValue('1.5');
+ });
+
+ it('should allow decimal input via keyboard', async () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0, max: 10 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [null, null] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ const fromInput = inputs[0];
+
+ userEvent.clear(fromInput);
+ userEvent.type(fromInput, '2.5');
+ userEvent.tab();
Review Comment:
Not applicable here — we're on `@testing-library/user-event ^12.8.3` (per
`package.json`), where `userEvent.clear/type/tab` are synchronous and don't
need `await`. The separate `async` test at line 414 uses `await` as
forward-compatibility hygiene; these older tests follow the synchronous v12 API.
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -234,6 +234,40 @@ export default function RangeFilterPlugin(props:
PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
Review Comment:
Already addressed — the current `calculateStep` avoids string parsing
entirely and uses `Math.log10` + `Math.round` (numerically stable) for all
ranges. The code comment calls this out explicitly.
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -234,6 +234,40 @@ export default function RangeFilterPlugin(props:
PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
+ // Use a step that gives approximately 100 steps across the range
+ return Math.pow(10, -Math.min(decimalPlaces + 1, 6));
+ }
+ return 0.01;
+ }
+
+ // For larger ranges, calculate step to give approximately 100-1000 steps
+ const idealSteps = 100;
+ let step = range / idealSteps;
+
+ // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
+ const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
+ step = Math.round(step / magnitude) * magnitude;
+
+ return step;
+ }, []);
Review Comment:
Already addressed — `calculateStep` now uses a single
`Math.log10`/`Math.round`-based approach for all ranges, eliminating the
special-case inconsistency.
--
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]
