Skip to content

Conversation

@MayaKirova
Copy link
Contributor

@MayaKirova MayaKirova commented Jun 18, 2020

Related to #5486

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@MayaKirova MayaKirova marked this pull request as ready for review June 18, 2020 14:41
@MayaKirova MayaKirova added the ❌ status: awaiting-test PRs awaiting manual verification label Jun 18, 2020
@VMihalkov VMihalkov added 💥 status: in-test PRs currently being tested ✅ status: verified Applies to PRs that have passed manual verification and removed ❌ status: awaiting-test PRs awaiting manual verification 💥 status: in-test PRs currently being tested labels Jun 19, 2020
@ChronosSF ChronosSF requested a review from skrustev June 22, 2020 08:09
const isPercentageWidth = colWidth && typeof colWidth === 'string' && colWidth.indexOf('%') !== -1;
if (isPercentageWidth) {
this._calcWidth = parseInt(colWidth, 10) / 100 * (grid.calcWidth - grid.featureColumnsWidth());
this._calcWidth = parseInt(colWidth, 10) / 100 * grid.calcWidth;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you remove the feature columns width in this case? In this case the user will need to take into consideration the row selection column for example, because his defined 100% column widths will render a scrollbar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skrustev When width is in % and the related flex styles are applied (flex-basis, min/max width etc.) they are resolved based on the whole container width. So the actual rendered element widths will be the column percentage width * the grid container width.

The previous calculation was wrong as it did not reflect the size of the % width elements as rendered in the DOM. This was not as apparent before since the calcWidth was only used for internal horizontal virtualization calculations. However since it now applies the size for mch header elements the misalignment between the header and content cells is more obvious (you can refer this test - 'Should calculate column width when a column has width in % and row selectors are enabled' , which reflects this scenario and shows the misalignment).

This should also fix the following scenario:
https://stackblitz.com/edit/angular-dba7e7?file=src/app/grid/grid-sample-selection/grid-selection.component.html
Where all columns have widths in % and due to the row selector the virtualization calculations are wrong and the last column is cut off.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I guess in the context that columns are sized based on the whole grid size and not scrollable area it is fine, since I saw that other features also treat it that way.

@ChronosSF ChronosSF merged commit b1abcc0 into master Jun 23, 2020
@ChronosSF ChronosSF deleted the mkirova/mch-percentage branch June 23, 2020 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

grid: multi-column-headers version: 10.0.x ✅ status: verified Applies to PRs that have passed manual verification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants