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

Bugfix/datagrid incremental loading fix #4892

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

filipwa84
Copy link
Contributor

@filipwa84 filipwa84 commented Jun 2, 2023

Fixes #4891

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

When using a collection that supports incremental loading, and the implementation of ISupportIncrementalLoading allows you to specify how many rows are retrieved on each load, then the DataGrid won't continue to load more data after the first set of data is retrieved if the height of the rows it receives is smaller than the available space for cells in the DataGrid.

The only way to get the DataGrid to load more data is to resize the window so that the available cell space is smaller than the total cell height.

What is the new behavior?

After loading the first "page", the DataGrid will continue to load more data while the CellsHeights property is larger than the total height of all visible cells.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • Tested code with current supported SDKs
  • Contains NO breaking changes

Other information

@ghost
Copy link

ghost commented Jun 2, 2023

Thanks filipwa84 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker and azchohfi June 2, 2023 14:13
@ghost ghost added the bug 🐛 An unexpected issue that highlights incorrect behavior label Jun 2, 2023
@filipwa84
Copy link
Contributor Author

@azchohfi Any idea when this one could be approved? It is just a one-liner so should be fairly straight forward.

@azchohfi
Copy link
Contributor

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Sorry @filipwa84 we're not focusing on the existing repo as we're preparing everything for the upcoming 8.0 release in the new repository https://github.com/CommunityToolkit/Windows. DataGrid isn't part of our initial migration story there, we've built a light-weight new version in Labs for core-scenarios: CommunityToolkit/Labs-Windows#415

version.json Outdated
@@ -1,5 +1,5 @@
{
"version": "7.1.3-build.{height}",
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't modify the version.json file here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where that change came from, I'll remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, will make it easier to integrate this and migrate the DataGrid in the future. I'll try and look more into that once we ship our upcoming release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change to version.json has been reverted

@filipwa84
Copy link
Contributor Author

filipwa84 commented Jul 14, 2023

Sorry @filipwa84 we're not focusing on the existing repo as we're preparing everything for the upcoming 8.0 release in the new repository https://github.com/CommunityToolkit/Windows. DataGrid isn't part of our initial migration story there, we've built a light-weight new version in Labs for core-scenarios: CommunityToolkit/Labs-Windows#415

Does that mean that bug fixes for the DataGrid won't be merged?

@michael-hawker
Copy link
Member

@filipwa84 we'll probably merge them still before we were to move any code - just not that it's a priority before we release out of the new repo, so may just take a bit still to get to, maybe a month?

(We haven't done the best job of that with some other PRs here, there's been a lot of moving pieces that are hard to keep track of.)

Test case(s) and reviews from anyone that engages with us would help any expedition, just our core team is focused on the new release.

@filipwa84
Copy link
Contributor Author

@michael-hawker Are you able to see why the Toolkit-CI started failing after I reverted the changes to version.json? I don't have access to the build log as far as I can tell.

@filipwa84
Copy link
Contributor Author

@michael-hawker @azchohfi any update on this? I'm not sure why it is not building though, It builds fine locally and there's only one line of code that has been changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior
Projects
None yet
3 participants