Skip to content

Commit

Permalink
fix(barSeries): error rendering bars with negative log scale (#2407)
Browse files Browse the repository at this point in the history
This PR mainly fixes the rendering of bars on a negative log scale. Also fixes the tooltip for banded bar charts.

In addition this PR fixes some other issues including
- Banded bar charts missing lower bound series in tooltip and legend
- Wrong highlighting of tooltips value and highlighted bar geometries, particularly for banded bar charts.
- Banded series items in tooltip were being shuffled according to the distance to the cursor position. Now I pre-sort them to not abruptly switch.
- The Chrome bug from #1053 associated with filling areas containing sharp corners, is now fixed and since using the shared `getY1ScaledValueFn` and `getY0ScaledValueFn` functions I just removed this logic to avoid checking.
  • Loading branch information
nickofthyme authored May 7, 2024
1 parent da0b921 commit 4ab6d8f
Show file tree
Hide file tree
Showing 22 changed files with 2,722 additions and 166 deletions.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 6 additions & 0 deletions e2e/tests/bar_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,4 +256,10 @@ test.describe('Bar series stories', () => {
);
});
});

test('should not render min bar heights for log scale values at baseline', async ({ page }) => {
await common.expectChartAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/bar-chart--min-height&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-Banded=true&knob-Custom No Results message=No Results&knob-Nice y ticks=true&knob-Scale Type=linear&knob-Series Type=bar&knob-Show positive data=true&knob-Split=true&knob-logMinLimit=1&knob-minBarHeight=5&knob-scale=log&knob-scaleType=log',
);
});
});
24 changes: 24 additions & 0 deletions e2e/tests/test_cases_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,28 @@ test.describe('Test cases stories', () => {
);
});
});

test.describe('Log scales', () => {
test('should correctly render negative values from baseline when banded', async ({ page }) => {
await common.expectChartAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/test-cases--log-with-negative-values&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-Show legend=&knob-Scale Type=log&knob-Series Type=bar&knob-logMinLimit=1&knob-Nice y ticks=&knob-Banded=true&knob-Split=&knob-Stacked=false&knob-Show positive data=',
);
});

test('should correctly render tooltip values for banded bars', async ({ page }) => {
await common.expectChartWithMouseAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/test-cases--log-with-negative-values&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-Show legend=&knob-Scale Type=log&knob-Series Type=bar&knob-logMinLimit=1&knob-Nice y ticks=&knob-Banded=true&knob-Split=&knob-Stacked=false&knob-Show positive data=',
{
top: 240,
right: 240,
},
);
});

test('should correctly render negative values from baseline when stacked', async ({ page }) => {
await common.expectChartAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/test-cases--log-with-negative-values&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-Show legend=&knob-Scale Type=log&knob-Series Type=bar&knob-logMinLimit=1&knob-Nice y ticks=&knob-Banded=&knob-Split=true&knob-Stacked=true&knob-Show positive data=',
);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { InitStatus, getInternalIsInitializedSelector } from '../../../../state/
import { isBrushingSelector } from '../../../../state/selectors/is_brushing';
import { getColorFromVariant, Rotation } from '../../../../utils/common';
import { Dimensions } from '../../../../utils/dimensions';
import { isPointGeometry, IndexedGeometry, PointGeometry } from '../../../../utils/geometry';
import { isPointGeometry, IndexedGeometry, PointGeometry, isBarGeometry } from '../../../../utils/geometry';
import { LIGHT_THEME } from '../../../../utils/themes/light_theme';
import { HighlighterStyle } from '../../../../utils/themes/theme';
import { computeChartDimensionsSelector } from '../../state/selectors/compute_chart_dimensions';
Expand Down Expand Up @@ -57,11 +57,20 @@ class HighlighterComponent extends React.Component<HighlighterProps> {
static displayName = 'Highlighter';

render() {
const { highlightedGeometries, chartDimensions, chartRotation, chartId, zIndex, isBrushing, style } = this.props;
const { chartDimensions, chartRotation, chartId, zIndex, isBrushing, style } = this.props;
if (isBrushing) return null;
const clipWidth = [90, -90].includes(chartRotation) ? chartDimensions.height : chartDimensions.width;
const clipHeight = [90, -90].includes(chartRotation) ? chartDimensions.width : chartDimensions.height;
const clipPathId = `echHighlighterClipPath__${chartId}`;

const seenBarSeries = new Set<string>();
const highlightedGeometries = this.props.highlightedGeometries.filter((geom) => {
if (!isBarGeometry(geom)) return true;
const seen = seenBarSeries.has(geom.seriesIdentifier.key);
seenBarSeries.add(geom.seriesIdentifier.key);
return !seen;
});

return (
<svg className="echHighlighter" style={{ zIndex }}>
<defs>
Expand Down Expand Up @@ -100,11 +109,12 @@ class HighlighterComponent extends React.Component<HighlighterProps> {
</g>
);
}

return (
<rect
key={i}
x={x}
y={y}
y={geom.y}
width={geom.width}
height={geom.height}
transform={geomTransform}
Expand Down
Loading

0 comments on commit 4ab6d8f

Please sign in to comment.