Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(grid): Fix calculate height when initially grid has no data #3950 #3972

Merged
merged 35 commits into from
May 16, 2019

Conversation

mpavlinov
Copy link
Contributor

@mpavlinov mpavlinov commented Feb 21, 2019

Closes #3950
Closes #3949
Closes #4745

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
  • This PR includes API docs for newly added methods/properties
  • 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
  • This PR includes behavioral changes and the feature specification has been updated with them

MayaKirova
MayaKirova previously approved these changes Feb 21, 2019
dkamburov
dkamburov previously approved these changes Feb 21, 2019
@mpavlinov mpavlinov dismissed stale reviews from dkamburov and MayaKirova via 6f70541 February 21, 2019 17:23
@MayaKirova MayaKirova added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Feb 25, 2019
@MayaKirova
Copy link
Contributor

@mpavlinov This affects the HierarchicalGrid. In master by default if no height is specified for rowIslands in remote data scenario, they extend to their full content.
With this change now shrink to about 100px:
image

This will probably affect existing samples.
Perhaps we should add special handling for child grids, so that by default when no height is set it will act as height = null instead of height = 100% since child containers are small by default and won't look good in the default use-case.

@MayaKirova MayaKirova added ❌ status: not-fixed and removed 💥 status: in-test PRs currently being tested labels Feb 25, 2019
@mpavlinov mpavlinov added the 🛠️ status: in-development Issues and PRs with active development on them label Feb 28, 2019
@mpavlinov mpavlinov added ❌ status: awaiting-test PRs awaiting manual verification and removed 🛠️ status: in-development Issues and PRs with active development on them labels May 9, 2019
@@ -123,7 +123,7 @@
<div class="igx-grid__tfoot-thumb" [hidden]='!hasVerticalSroll()' [style.height.px]='summariesHeight' [style.width.px]="scrollWidth"></div>
</div>

<div class="igx-grid__scroll" [style.height]="'18px'" #scr [hidden]="unpinnedWidth - totalWidth >= 0">
<div class="igx-grid__scroll" [style.height]="'18px'" #scr [hidden]="unpinnedWidth - (totalWidth - 18) >= 0">
Copy link
Member

Choose a reason for hiding this comment

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

This seems to allow the vertical scrollbar to cover the last cells right edge before a horizontal scrollbar is shown. Why is this part of this PR?

I believe we moved the v scrollbar out of the tbody so that it never covers the cells.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChronosSF There were failing tests. The issues was with the horizontal scroll that opened up a gap on the bottom. I fixed it by clearing the cache when the columns get recalculated.
Note: The issue is when the columns don't have widths set.

@ChronosSF
Copy link
Member

ChronosSF commented May 9, 2019

@mpavlinov , if you don't have empty data but rather have initial data that can fit under what height 100% would usually be and then load the full data, you still get the grid to disable virtualization. I guess we need to derive the height on the data prop setter as well and not lose the initial setting. Not sure if this won't break something else though.

@ChronosSF ChronosSF added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels May 9, 2019
@mpavlinov mpavlinov added 🛠️ status: in-development Issues and PRs with active development on them and removed 💥 status: in-test PRs currently being tested labels May 13, 2019
@mpavlinov mpavlinov added ❌ status: awaiting-test PRs awaiting manual verification and removed 🛠️ status: in-development Issues and PRs with active development on them labels May 16, 2019
@ChronosSF ChronosSF self-requested a review May 16, 2019 12:18
@ChronosSF ChronosSF added ✅ status: verified Applies to PRs that have passed manual verification and removed ❌ status: awaiting-test PRs awaiting manual verification labels May 16, 2019
@mpavlinov mpavlinov added the squash-merge Merge PR with "Squash and Merge" option label May 16, 2019
@ChronosSF ChronosSF merged commit 847ab53 into 7.2.x May 16, 2019
@ChronosSF ChronosSF deleted the mpavlov/issue-3950-master branch May 16, 2019 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grid: general squash-merge Merge PR with "Squash and Merge" option version: 7.2.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