Add Auto Column Size #18549
Add Auto Column Size #18549
Conversation
cssuh
requested review from
kburtram,
aasimkhan30,
caohai,
laurennat,
lewis-sanchez and
Benjin
as code owners
PR Changes
|
|
Can you add a video of the double-click to expand a column to the PR description? |
|
Last week, we'd talked about it remembering when a user adjusts a column width for that result set if they rerun the query. Is that part of this PR? If so, can you add a short video of how that works? (btw, separate video per topic/feature is best) |
| @@ -164,6 +166,13 @@ export class Table<T extends Slick.SlickData> implements IThemable { | |||
| new CopyKeybind(this.uri, this.resultSetSummary, this.webViewState), | |||
| ); | |||
|
|
|||
| this.registerPlugin( | |||
| new AutoColumnSize({ | |||
| maxWidth: MAX_COLUMN_WIDTH_PX, | |||
There was a problem hiding this comment.
A user can still manually adjust wider than this MAX_COLUMN_WIDTH_PX, right?
There was a problem hiding this comment.
No, I thought we discussed that we wouldn't include a user option for this because we don't want to add too many config options. Should I change this?
There was a problem hiding this comment.
No, you're right about that. What I meant was that if I have a column with lots of text such that the content is wider than MAX_COLUMN_WIDTH_PX, autosize will limit itself to MAX_COLUMN_WIDTH_PX, but I - as the user - will still be able to drag the column to be even wider to see more of my data, right?
This part slipped my mind, I'll add it shortly. |
| let data = this._grid.getData() as Slick.DataProvider<T>; | ||
| let viewPort = this._grid.getViewport(); | ||
| let start = Math.max(0, viewPort.top); | ||
| let end = Math.min(data.getLength(), viewPort.bottom); |
There was a problem hiding this comment.
are we going through all the data in the result set, or the visible viewPort? I'm not sure what viewPort.bottom is. But we probably shouldn't go through the whole result set, maybe 100 or 1000 rows is totally fine to get a general shape of the data.
There was a problem hiding this comment.
It just goes through the visible viewPort
There was a problem hiding this comment.
Recording here to make sure it doesn't get lost: the suggestion from sync yesterday was to calculate the width using the first 50-100 rows of data, rather than just the viewport since the viewport is possibly only a small slice.
| let max = 0, | ||
| maxTemplate: JQuery | HTMLElement | string | undefined; | ||
| let formatFun = columnDef.formatter; | ||
| texts.forEach((text, index) => { |
There was a problem hiding this comment.
this looks expensive at scale (e.g. lots of rows)
There was a problem hiding this comment.
@kburtram Did some perf testing on this, and since it just goes by the visible viewport, it only did the first 10 rows of each column for me (30 rows if I expand the grid to my full screen). With 1024 columns (which seems like the upper recommended limit for # of columns), it took a couple seconds extra compared to if I had the setting off.
There was a problem hiding this comment.
Can you add a telemetry point with timing and number of columns here so that we can detect any egregious times to autosize?
Also okay if that comes in a quick follow-up PR. |
@Benjin I think I'll add this in a follow-up PR, and I realized this is not for if they re-run the query (since that would generate a new query result set and result set grid), but for if they leave the result pane and then come back (like switching to messages tab and then returning) Created #18554 to track |
Gives the proper space for column widths. Also enables double clicking on a column border to snap to the correct size.
In the first screenshot you can see that the column headers have an ellipsis but in the second screenshot, they are sized correctly.
Before:
After:
Note: Instead of adding an additional max column size config option, I hardcoded it at 400px to minimize the number of settings options we have for this extension.
Double-click feature:
Note: this feature will expand the column to fit the header or any visible row data (whichever is larger), but won't shrink a cell that's already been manually expanded past the size of the header or visible row data.
Fixes #18089
Fixes #18534