Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Columns not auto sizing correctly #2359

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/grid/src/CellRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,16 @@ abstract class CellRenderer {
context: CanvasRenderingContext2D,
text: string,
width: number,
fontWidth: number,
fontWidthLower?: number,
fontWidthUpper?: number,
truncationChar?: string
): string =>
GridRenderer.truncateToWidth(
context,
text,
width,
fontWidth,
fontWidthLower,
fontWidthUpper,
truncationChar
),
{ max: 10000 }
Expand Down
11 changes: 7 additions & 4 deletions packages/grid/src/DataBarCellRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
import GridColorUtils from './GridColorUtils';
import GridUtils from './GridUtils';
import memoizeClear from './memoizeClear';
import { DEFAULT_FONT_WIDTH, type GridRenderState } from './GridRendererTypes';
import { type GridRenderState } from './GridRendererTypes';
import type GridModel from './GridModel';

interface DataBarRenderMetrics {
Expand Down Expand Up @@ -87,7 +87,8 @@ class DataBarCellRenderer extends CellRenderer {
allRowHeights,
allRowYs,
firstColumn,
fontWidths,
fontWidthsLower,
fontWidthsUpper,
} = metrics;

const isFirstColumn = column === firstColumn;
Expand All @@ -103,13 +104,15 @@ class DataBarCellRenderer extends CellRenderer {
width: textWidth,
} = GridUtils.getTextRenderMetrics(state, column, row);

const fontWidth = fontWidths?.get(context.font) ?? DEFAULT_FONT_WIDTH;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see in a lot of places that we no longer check the default font. What happens if the font is not the map? Do we get a default somehow, or is it guaranteed to be calculated?

Copy link
Contributor Author

@ericlln ericlln Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that this constant was being used in a lot of different places, since CellRenderer.getCachedTruncatedString doesn’t accept an undefined value for the font widths. The function GridRenderer.truncateToWidth that it calls does though, and already uses that constant for default values, so I made getCachedTruncatedString accept undefined for the font widths, and the constant can be removed from other places safely.

If you did happen to need the font widths for something else in those methods, you would have to add back that undefined check.

const fontWidthLower = fontWidthsLower.get(context.font);
const fontWidthUpper = fontWidthsUpper.get(context.font);
const truncationChar = model.truncationCharForCell(modelColumn, modelRow);
const truncatedText = this.getCachedTruncatedString(
context,
text,
textWidth,
fontWidth,
fontWidthLower,
fontWidthUpper,
truncationChar
);

Expand Down
90 changes: 89 additions & 1 deletion packages/grid/src/GridMetricCalculator.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { trimMap } from './GridMetricCalculator';
import GridMetricCalculator, { trimMap } from './GridMetricCalculator';

describe('trimMap', () => {
function makeMap(low = 0, high = 10): Map<number, number> {
Expand Down Expand Up @@ -41,3 +41,91 @@ describe('trimMap', () => {
expectResult(makeMap(0, 100), makeMap(51, 100), 100, 50);
});
});

describe('getTextWidth', () => {
const font = 'mock font';
const mockCharWidths = new Map([
['a', 10],
['b', 20],
['c', 30],
['d', 40],
['e', 50],
]);
let gridMetricCalculator;
let allCharWidths;

beforeEach(() => {
gridMetricCalculator = new GridMetricCalculator();
allCharWidths = new Map();
Object.assign(gridMetricCalculator, {
allCharWidths,
});
});

it('should calculate text width and cache char widths if not in cache', () => {
const input = 'abc';
const mockContext = {
measureText: jest.fn().mockImplementation(char => ({
width: mockCharWidths.get(char),
})),
};

const textWidth = gridMetricCalculator.getTextWidth(
mockContext,
font,
input
);

const charWidths = allCharWidths.get(font);
expect(charWidths.get('a')).toEqual(10);
expect(charWidths.get('b')).toEqual(20);
expect(charWidths.get('c')).toEqual(30);
expect(textWidth).toEqual(60);
});

it('should calculate text width using cached char widths if in cache', () => {
const input = 'abcd';
const mockContext = {
measureText: jest.fn().mockImplementation(char => ({
width: mockCharWidths.get(char),
})),
};
const dummyMockContext = {
measureText: jest.fn(),
};

const firstRun = gridMetricCalculator.getTextWidth(
mockContext,
font,
input
);
const secondRun = gridMetricCalculator.getTextWidth(
dummyMockContext,
font,
input
);

expect(mockContext.measureText).toHaveBeenCalledTimes(input.length);
expect(dummyMockContext.measureText).toHaveBeenCalledTimes(0);
expect(firstRun).toEqual(100);
expect(secondRun).toEqual(100);
});

it('should terminate early if max width is exceeded', () => {
const input = 'abcde';
const mockContext = {
measureText: jest.fn().mockImplementation(char => ({
width: mockCharWidths.get(char),
})),
};

const textWidth = gridMetricCalculator.getTextWidth(
mockContext,
font,
input,
50
);

expect(textWidth).toEqual(50);
});
});
Loading
Loading