Further mark density bugfixes

Author: KoolADE85

components/dash-core-components/package-lock.json

@@ -78,7 +78,7 @@
     "@plotly/webpack-dash-dynamic-import": "^1.3.0",
     "@types/d3-format": "^3.0.4",
     "@types/fast-isnumeric": "^1.1.2",
-    "@types/jest": "^29.5.0",
+    "@types/jest": "^29.5.14",
     "@types/js-search": "^1.4.4",
     "@types/ramda": "^0.31.0",
     "@types/react": "^16.14.8",

components/dash-core-components/package.json

@@ -33,59 +33,50 @@ const alignDecimalValue = (v: number, d: number) =>
 const alignValue = (v: number, d: number) =>
     decimalCount(d) < 1 ? alignIntValue(v, d) : alignDecimalValue(v, d);
 
+export const applyD3Format = (mark: number, min: number, max: number) => {
+    const mu_ten_factor = -3;
+    const k_ten_factor = 4; // values < 10000 don't get formatted
+
+    const ten_factor = Math.log10(Math.abs(mark));
+    if (
+        mark === 0 ||
+        (ten_factor > mu_ten_factor && ten_factor < k_ten_factor)
+    ) {
+        return String(mark);
+    }
+    const max_min_mean = (Math.abs(max) + Math.abs(min)) / 2;
+    const si_formatter = formatPrefix(',.0', max_min_mean);
+    return String(si_formatter(mark));
+};
+
 const estimateBestSteps = (
     minValue: number,
     maxValue: number,
     stepValue: number,
     sliderWidth?: number | null
 ) => {
-    let targetMarkCount = 11; // Default baseline
-
-    // Scale mark density based on slider width
-    if (sliderWidth) {
-        const baselineWidth = 330;
-        const baselineMarkCount = 11; // 0, 10, 20, 30, 40, 50, 60, 70, 80, 90, 100
-
-        // Calculate density multiplier based on width
-        const widthMultiplier = sliderWidth / baselineWidth;
-
-        // Target mark count scales with width but maintains consistent density
-        // The range adjustment should be removed - we want consistent mark density based on width
-        targetMarkCount = Math.round(baselineMarkCount * widthMultiplier);
-
-        // Ensure reasonable bounds
-        const UPPER_BOUND = 50;
-        targetMarkCount = Math.max(3, Math.min(targetMarkCount, UPPER_BOUND));
-
-        // Adjust density based on maximum character width of mark labels
-        // Estimate the maximum characters in any mark label
-        const maxValueChars = Math.max(
-            String(minValue).length,
-            String(maxValue).length,
-            String(Math.abs(minValue)).length,
-            String(Math.abs(maxValue)).length
-        );
-
-        // Baseline: 3-4 characters (like "100", "250") work well with baseline density
-        // For longer labels, reduce density to prevent overlap
-        const baselineChars = 3.5;
-        if (maxValueChars > baselineChars) {
-            const charReductionFactor = baselineChars / maxValueChars;
-            targetMarkCount = Math.round(targetMarkCount * charReductionFactor);
-            targetMarkCount = Math.max(2, targetMarkCount); // Ensure minimum of 2 marks
-        }
-    }
+    // Use formatted label length to account for SI formatting
+    // (e.g. labels that look like "100M" vs "100000000")
+    const formattedMin = applyD3Format(minValue, minValue, maxValue);
+    const formattedMax = applyD3Format(maxValue, minValue, maxValue);
+    const maxValueChars = Math.max(formattedMin.length, formattedMax.length);
+
+    // Calculate required spacing based on label width
+    // Estimate: 10px per character + 20px margin for spacing between labels
+    // This provides comfortable spacing to prevent overlap
+    const pixelsPerChar = 10;
+    const spacingMargin = 20;
+    const minPixelsPerMark = maxValueChars * pixelsPerChar + spacingMargin;
+
+    const effectiveWidth = sliderWidth || 330;
+
+    // Calculate maximum number of marks that can fit without overlap
+    let targetMarkCount = Math.floor(effectiveWidth / minPixelsPerMark) + 1;
+    targetMarkCount = Math.max(3, Math.min(targetMarkCount, 50));
 
     // Calculate the ideal interval between marks based on target count
     const range = maxValue - minValue;
-    let idealInterval = range / (targetMarkCount - 1);
-
-    // Check if the step value is fractional and adjust density
-    if (stepValue % 1 !== 0) {
-        // For fractional steps, reduce mark density by half to avoid clutter
-        targetMarkCount = Math.max(3, Math.round(targetMarkCount / 2));
-        idealInterval = range / (targetMarkCount - 1);
-    }
+    const idealInterval = range / (targetMarkCount - 1);
 
     // Calculate the multiplier needed to get close to idealInterval
     // Round to a "nice" number for cleaner mark placement
@@ -102,13 +93,13 @@ const estimateBestSteps = (
         niceMultiplier = 2;
     } else if (normalized <= 3.5) {
         niceMultiplier = 2.5;
-    } else if (normalized <= 7.5) {
+    } else if (normalized <= 5) {
         niceMultiplier = 5;
     } else {
         niceMultiplier = 10;
     }
 
-    const bestMultiplier = niceMultiplier * magnitude;
+    const bestMultiplier = Math.max(1, niceMultiplier * magnitude);
     const bestInterval = stepValue * bestMultiplier;
 
     // All marks must be at valid step positions: minValue + (n * stepValue)
@@ -174,31 +165,16 @@ export const setUndefined = (
     return definedMarks;
 };
 
-export const applyD3Format = (mark: number, min: number, max: number) => {
-    const mu_ten_factor = -3;
-    const k_ten_factor = 3;
-
-    const ten_factor = Math.log10(Math.abs(mark));
-    if (
-        mark === 0 ||
-        (ten_factor > mu_ten_factor && ten_factor < k_ten_factor)
-    ) {
-        return String(mark);
-    }
-    const max_min_mean = (Math.abs(max) + Math.abs(min)) / 2;
-    const si_formatter = formatPrefix(',.0', max_min_mean);
-    return String(si_formatter(mark));
-};
-
 export const autoGenerateMarks = (
     min: number,
     max: number,
     step?: number | null,
     sliderWidth?: number | null
 ) => {
     const marks = [];
-    // Always use dynamic logic, but pass the provided step as a constraint
-    const effectiveStep = step || calcStep(min, max, 0);
+
+    const effectiveStep = step ?? 1;
+
     const [start, interval, chosenStep] = estimateBestSteps(
         min,
         max,

components/dash-core-components/src/utils/computeSliderMarkers.ts

@@ -42,7 +42,7 @@ const areAllMarksValidSteps = (
 
 describe('Dynamic Slider Mark Density', () => {
     describe('Baseline behavior (330px width)', () => {
-        test('should show ~10-11 marks for 0-100 range with step=5', () => {
+        test('should show appropriate marks for 0-100 range with step=5 at 330px', () => {
             const marks = sanitizeMarks({
                 min: 0,
                 max: 100,
@@ -54,7 +54,9 @@ describe('Dynamic Slider Mark Density', () => {
             expect(marks).toBeDefined();
             const positions = getMarkPositions(marks);
 
-            expect(positions.length).toBe(11);
+            // With pixel-based algorithm: 3-char labels need ~50px per mark
+            // 330px / 50px = ~7 marks (plus min/max adjustment gives 8)
+            expect(positions.length).toBe(8);
             expect(areAllMarksValidSteps(marks, 0, 5)).toBe(true);
             expect(positions).toContain(0);
             expect(positions).toContain(100);
@@ -220,36 +222,17 @@ describe('Dynamic Slider Mark Density', () => {
             });
         });
 
-        test('should reduce density by half for fractional steps', () => {
-            const integerStep = sanitizeMarks({
-                min: 0,
-                max: 10,
-                marks: undefined,
-                step: 1, // Integer step
-                sliderWidth: 330,
-            });
-
+        test('should have appropriate density for fractional steps', () => {
             const fractionalStep = sanitizeMarks({
                 min: 0,
                 max: 10,
                 marks: undefined,
                 step: 0.5, // Fractional step
-                sliderWidth: 330,
+                sliderWidth: 1600,
             });
 
-            const integerPositions = getMarkPositions(integerStep);
             const fractionalPositions = getMarkPositions(fractionalStep);
-
-            // Fractional step should have roughly half the marks of integer step
-            expect(fractionalPositions.length).toBeLessThan(
-                integerPositions.length
-            );
-            expect(fractionalPositions.length).toBeLessThanOrEqual(
-                Math.ceil(integerPositions.length / 2) + 1
-            );
-
-            // Both should be valid steps
-            expect(areAllMarksValidSteps(integerStep, 0, 1)).toBe(true);
+            expect(fractionalPositions.length).toBe(21);
             expect(areAllMarksValidSteps(fractionalStep, 0, 0.5)).toBe(true);
         });
 
@@ -413,5 +396,28 @@ describe('Dynamic Slider Mark Density', () => {
             expect(positions[0]).toBe(0);
             expect(positions[positions.length - 1]).toBe(1000000000);
         });
+
+        test('does not have all marks labeled as "2k" for range 1952 to 2007', () => {
+            const marks = sanitizeMarks({
+                min: 1952,
+                max: 2007,
+                marks: undefined,
+                step: undefined,
+                sliderWidth: 365,
+            });
+
+            // Get all the label values (not the keys)
+            const labels = Object.values(marks);
+
+            // Count unique labels
+            const uniqueLabels = new Set(labels);
+
+            // Should have more than one unique label
+            expect(uniqueLabels.size).toBeGreaterThan(1);
+
+            // Should NOT have all labels be "2k"
+            const allLabels2k = labels.every(label => label === '2k');
+            expect(allLabels2k).toBe(false);
+        });
     });
 });