fix: Columns not auto sizing correctly#2359
Conversation
Now that the column sizing is more precise, the sort arrow is too close to the header text
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2359 +/- ##
==========================================
+ Coverage 46.76% 46.80% +0.03%
==========================================
Files 710 710
Lines 39107 39141 +34
Branches 9773 9956 +183
==========================================
+ Hits 18288 18318 +30
- Misses 20808 20812 +4
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
causes all snapshots to be updated which makes it hard to tell whether the difference is caused by the theme change or other changes should be add padding in between the header text and sort arrow in a different way anyway
|
|
||
| context.font = font; | ||
| // Assume char `m` is the largest character | ||
| const textMetrics = context.measureText('m'); |
There was a problem hiding this comment.
In the ticket, the complaint was about a capital 'M' overflowing the width. Is lowercase 'm' going to have the same width?
There was a problem hiding this comment.
When I was initially investigating this issue I measured all alphanumeric characters using our canvas context, and found that a lowercase ‘m’ is a wider than an upper case one by about 8%, so I ended up taking that as the largest value. This might not be true for all fonts but it seems to be more common based on my limited research.
| width: textWidth, | ||
| } = GridUtils.getTextRenderMetrics(state, column, row); | ||
|
|
||
| const fontWidth = fontWidths?.get(context.font) ?? DEFAULT_FONT_WIDTH; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Test snippet used to see if column width calculation works as expected; resize columns to observe truncation behaviour
Closes #2288