Add min-width on the columns #7557
Conversation
|
jcgueriaud1
changed the title
|
I guess the visual tests can't run from a fork |
| * cells have different font sizes. Instead, use `rem` if you need | ||
| * a length unit relative to the font size. | ||
| */ | ||
| minWidth: string | null | undefined; |
There was a problem hiding this comment.
nit: space missing after this line
| /** @private */ | ||
| _minWidthChanged(minWidth) { | ||
| if (this.parentElement && this.parentElement._columnPropChanged) { | ||
| this.parentElement._columnPropChanged('minWidth'); |
| // update the columns without autowidth and set the min width | ||
| const noAutoWidthCols = this._getColumns().filter((col) => !col.hidden && !col.autoWidth); | ||
|
|
||
| noAutoWidthCols.forEach((col) => { | ||
| const calculatedWidth = col.width; | ||
| if (col.minWidth) { | ||
| col.width = `max(${col.minWidth}, ${calculatedWidth})`; | ||
| } else { | ||
| col.width = `${calculatedWidth}`; | ||
| } | ||
| }); |
| @@ -0,0 +1,3 @@ | |||
| import '../theme/lumo/lit-all-imports.js'; | |||
| import '../src/lit-all-imports.js'; | |||
There was a problem hiding this comment.
Could align with the polymer test and import the styled grid (import '../lit-all-imports.js';)
| }); | ||
| } | ||
|
|
||
| describe('column auto-width', () => { |
There was a problem hiding this comment.
Is there a reason to have all min-width -specific tests in this separate test file instead of including, for example, the auto-width test to column-auto-width.common.js (under "min-width" suite where necessary)?
As an example, adding the following (adjusted) test from this PR to column-resizing.common.js right under "should resize the last non-hidden child column" would save us from replicating the same test setup here:
it('should resize the child columns with min-width', () => {
const columns = grid.querySelectorAll('vaadin-grid-column');
columns[0].minWidth = '120px';
columns[1].minWidth = '70px';
const headerRows = getRows(grid.$.header);
const groupCell = getRowCells(headerRows[0])[0];
const handle = groupCell.querySelector('[part~="resize-handle"]');
const cell = getRowCells(headerRows[1])[1];
const rect = cell.getBoundingClientRect();
const options = { node: handle };
expect(cell.clientWidth).to.equal(100);
fire('track', { state: 'start' }, options);
fire('track', { state: 'track', x: rect.right + (direction === 'rtl' ? -50 : 50), y: 0 }, options);
expect(cell.clientWidth).to.equal(direction === 'rtl' ? 70 : 150);
expect(groupCell.clientWidth).to.equal(direction === 'rtl' ? 190 : 270);
});There was a problem hiding this comment.
For the resizing, I think there is no particular reason. For the other tests, they are using expectColumnWidthsToBeOk with a regexp.
auto-width is not what I'm testing that's the reason it's not in the auto-width test file.
For example, I need to add at least a test case without auto-width(that could be in an other file or in min-width).
Let me add all the needed tests then we can check if they need to be in the existing test files or in one specific file.
Description
Please list all relevant dependencies in this section and provide summary of the change, motivation and context.
Fixes vaadin/platform#6553
Type of change
Checklist
Additional for
Featuretype of changeOnly the
min-widthis implemented because the max-width with flex-grow are not working together if the columns are grouped. There is an example in html of the issue here: https://codepen.io/jcgueriaud/pen/JjQYBOQIf you have 2 columns
flex-grow:1then the column group hasflex-grow:2(1+1).But the max-width will "block" the column to grow at a certain point and the width of the group will be different.
Since the column doesn't have a
flex-shrinkproperty it's not an issue formin-width.To avoid flaky test, I'm parsing the width of the columns like
max(min-width, calculatedWidth)as a Regex and check the width with a delta.As an alternative of the implementation I tried:
max(min-width, calculatedWidth)I did the calculation in Javascript and set a result in pixel. But that it not working if themin-widthis inremor%Maybe we should refactor Grid to use css grid instead of flexbox. But it's probably to difficult as an external contributor.