feat(SchemaViewer): calculate column width based on data #1885
Conversation
| } | ||
|
|
||
| return rowTableColumns; | ||
| console.log(normalizeColumns(rowTableColumns, data)); |
| data: dataSlice, | ||
| name: column.name, | ||
| header: typeof column.header === 'string' ? column.header : undefined, | ||
| sortable: column.sortable === undefined, |
There was a problem hiding this comment.
sortable could be true, I'd just pass column.sortable here
| if ( | ||
| maxColumnContentLength * PIXELS_PER_CHARACTER + HEADER_PADDING >= | ||
| MAX_COLUMN_WIDTH |
There was a problem hiding this comment.
sortPadding is not used here
src/utils/getColumnWidth.ts
Outdated
| function hasProperty(obj: KeyValueRow | SchemaData, key: string): obj is KeyValueRow { | ||
| return key in obj; | ||
| } |
There was a problem hiding this comment.
This type guard actually implicitly do as KeyValueRow to any passed Object type value without any real check. Although it have no influence right now, it's generally not very good practice - it's harder to find error here than in explicit as type, since you expect checks in type guards. And it's less error proof in case we want to make some changes.
You can do following to get rid of any assertions:
- Set type
TtoRecord<string, unknown>(getColumnWidth<Record<string, unknown>>) - Convert
SchemaDatafrominterfacetotypeso there will be noIndex signature for type 'string' is missingerror
This will allow to get rid of hasProperty helper and also will make this function more general purpose, since it won't be with specific types.
There was a problem hiding this comment.
Genius! I've have had so much pain with these types yesterday.
There was a problem hiding this comment.
I found this difference between types and interfaces myself only yesterday. Here the issue, that you could read: microsoft/TypeScript#15300, and the most explanatory comment about it: microsoft/TypeScript#15300 (comment)
src/utils/getColumnWidth.ts
Outdated
| const sortPadding = | ||
| sortable && maxColumnContentLength <= headerContentLength ? SORT_ICON_PADDING : 0; |
There was a problem hiding this comment.
There is a problem with such calculations. In the example your code will return 70, while it should return 78:
maxColumnContentLength = 5
headerContentLength = 4
sortable = true
sortPadding = 0
actualColumnContentLength = 10 * 5 + 20 // 70
actualHeaderContentLength = 10 * 4 + 18 + 20 // 78You may assume, that sort icon length is equal to 2 characters and just add this to headerContentLength - there will be 2px difference, you won't need to adjust you calculations for sortPadding
const headerContentLength = typeof header === 'string' ? header.length : name.length;
const headerContentLengthWithSort = sortable ? headerContentLength + 2 : headerContentLengthOr you may compare actual widths, but not length in characters
closes #1011
Stand
CI Results
Test Status: ❌ FAILED
📊 Full Report
😟 No changes in tests. 😕
Bundle Size: ✅
Current: 80.19 MB | Main: 80.19 MB
Diff: +2.23 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information