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.
|
Dear @jcgueriaud1 , there are some unresolved review comments. Are you planning to address those? |
|
No, I'm not planning to update this PR. In the current state of the PR, the changes between Maybe the way it's done is wrong and should be re-think differently. |
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.