fix(table): set min-width to <col> #5464
Conversation
🦋 Changeset detectedLatest commit: 644b141 The changes in this PR will be included in the next version bump. This PR includes changesets to release 54 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
- update and run tests relative to tables - fix types in TableView after removing @ts-nocheck
| } | ||
|
|
||
| nextDOM = nextDOM.nextSibling | ||
| if (nextDOM === null) { |
There was a problem hiding this comment.
Sorry for the nit, would prefer to keep this as
| if (nextDOM === null) { | |
| if (!nextDOM) { |
Just in case it is undefined, and to reduce differentiation from the prior behavior
| if (nextDOM === null) { | ||
| const colElement = document.createElement('col') | ||
|
|
||
| colElement.style.setProperty(...getColStyleDeclaration(cellMinWidth, hasWidth)) |
There was a problem hiding this comment.
While clever, I would prefer to be explicit around this, so that it is easy to understand what is happening:
| colElement.style.setProperty(...getColStyleDeclaration(cellMinWidth, hasWidth)) | |
| const [propertyKey, propertyValue] = getColStyleDeclaration(cellMinWidth, hasWidth) | |
| colElement.style.setProperty(propertyKey, propertyValue) |
Same thing in the next few lines
| export function getColStyleDeclaration(minWidth: number, width: number | undefined): [string, string] { | ||
| if (width) { | ||
| // apply the stored width unless it is below the configured minimum cell width | ||
| return ['width', `${Math.max(width, minWidth)}px`] | ||
| } | ||
|
|
||
| // set the minimum with on the column if it has no stored width | ||
| return ['min-width', `${minWidth}px`] | ||
|
|
||
| } | ||
|
|
||
| export function getColStyle(minWidth: number, width: number): string { | ||
| return getColStyleDeclaration(minWidth, width).join(': ') | ||
| } |
There was a problem hiding this comment.
This is abstracting over a couple of if statements, I'm not sure it warrants the complexity it adds.
I would honestly prefer for there to be a single function here, either stick to the string version or the array version, both just adds complexity and indirection.
There was a problem hiding this comment.
I extracted them because the logic was originally copy-pasted in both the create and the update code, I can remove the getColStyle function and do the concatenation in the table creation function directly instead of having a function in this file
|
I'll commit the changes when I get the time |
|
I forgot to run the tests and I had fuck up something in my code |
|
Is there anything I can do to get this PR merged ? It would allow me to remove some rather unsavory code from my codebase (i.e. copy-paste of the files changed by the PR) |
|
Hi @Ragnar-Oock, I hadn't realized this was ready yet. It seemed to me that some of your tests you wanted fix: #5464 (comment) |
|
Thanks for your contribution. |
Changes Overview
Add
min-widthto the style attribute of<col>elements whenwidthis not available (the user didn't resize column)Implementation Approach
Added a helper to generate the style of the
<col>elements from the stored column width and the configuredcellMinWidthTesting Done
editor.getHTMLin Table related suites<col>elements were generated with the appropriate style attributeVerification Steps
width: 100%set on the table=> The empty columns (or those with not enough content to push them above the minimum) respect the configured cellMinWidth
Additional Notes
I took the liberty of fixing up the types in the
TableView.tsfile after removing the// @ts-nocheckpresent at the top of the file. I have no objection to reverting the type changes if they are deemed out of scope for the PR.Checklist
Related Issues
#5435