Add support for fixed column widths#23
Conversation
texodus
left a comment
texodus
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I think the work so far here demonstrates we can't make a real improvement to this feature without first implementing good underlying abstractions for "user-draggable column resizing" (e.g. overrides) and "auto-detected column minimum widths" (e.g. auto), as 'fixed column widths' conflates both of these topics as well as "HTML layout generated widths" (e.g. offsetWidth). It may even be possible to use getComputedStyle() to read these bounds from the computed style rules on the element itself.
You might also try using your apply() function to set a class instead, then use a generated class and an !important directive to override the min-width and max-width generated by the auto-sizing.
patricio-mancini
force-pushed
the
fixed-max-width-for-columns-feature
branch
2 times, most recently
from
1baf5f7 to
738d697
Compare
patricio-mancini
force-pushed
the
fixed-max-width-for-columns-feature
branch
from
738d697 to
8e22ea4
Compare
texodus
left a comment
texodus
left a comment
There was a problem hiding this comment.
Looks great! Great test coverage and great documentation as well. I left some small comments inline below that are not necessary to address.
| * It is possible to manually shrink the column width up to the limit of 10 pixels. | ||
| * Column widths are calculated by the library using the max-width css rule. Which | ||
| means that settings the max-width from a css rule will lead to a fixed width | ||
| behavior for the cells of that column. |
There was a problem hiding this comment.
I believe these list-item hard wraps need to be indented https://www.markdownguide.org/basic-syntax/#adding-elements-in-lists
| // Set fixed min-width to cells when appropiate. | ||
| function styleListener() { | ||
| const ths = tableApi.querySelectorAll("thead th"); | ||
| const tds = tableApi.querySelectorAll("tbody td"); |
There was a problem hiding this comment.
This can be
const elems = tableApi.querySelectorAll("thead th, tbody td");The DOM API is very expressive already!
| /* Set fixed width for cells. */ | ||
| .fixed { | ||
| min-width: 100px !important; | ||
| max-width: 100px !important; |
There was a problem hiding this comment.
This seems important - I'd put it at the top!
|
|
||
| const baseColumns = ["Fixed", "Not Set"]; | ||
| const columnCount = 20; | ||
| const cellCount = 1000; |
There was a problem hiding this comment.
I think these are also CONSTANTS and belong at the top of the script
|
Thanks for the PR! |
2 participants
This pull request adds support for fixed column widths by applying custom attributes
maxWidthandminWidthto the cells of the<regular-table/>ensuring that the columns don’t grow or shrink past the attributes’ limits. Adds a new example:fixed_column_width.html, an example use of the fixed column width feature allowing developers to explore and extend its implementation.