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

Add EmptyContent parameter to Virtualize component #49185

Merged
merged 4 commits into from
Jul 7, 2023
Merged

Add EmptyContent parameter to Virtualize component #49185

merged 4 commits into from
Jul 7, 2023

Conversation

etemi
Copy link
Contributor

@etemi etemi commented Jul 3, 2023

Adds EmptyContent parameter to Virtualize component

Description

This PR adds the parameter "EmptyContent" to the Virtualize component as proposed in #28770.

Please note that this implementation, when used with an asynchronous ItemsProvider, will render the "EmptyContent" even though the ItemsProvider may not have completed (inital load). It's up to the user of the Virtualize component to "design" the EmptyContent template to show e.g. a progress indicator when data is loaded for the first time.

Feel free to adapt the code/docs as needed.

Fixes #28770

@etemi etemi requested a review from a team as a code owner July 3, 2023 15:59
@ghost ghost added area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member labels Jul 3, 2023
@ghost
Copy link

ghost commented Jul 3, 2023

Thanks for your PR, @etemi. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

Copy link
Member

@mkArtakMSFT mkArtakMSFT left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, @etemi.
@SteveSandersonMS can you please review this? Thanks!

@SteveSandersonMS
Copy link
Member

Thanks @etemi! This looks really good.

The one thing I think we should change is this:

Please note that this implementation, when used with an asynchronous ItemsProvider, will render the "EmptyContent" even though the ItemsProvider may not have completed (inital load)

I think that would be surprising and probably not what developers would want in most cases. Would it work to change this so we only show the "EmptyContent" when the grid is not loading and has zero items?

@etemi
Copy link
Contributor Author

etemi commented Jul 6, 2023

Thanks for your feedback @SteveSandersonMS!

I changed it so that "EmptyContent" is now only shown when the component is not loading and has zero items.

finally
{
_loading--;
}
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 what you're doing with the counter here, and think this could work, but also I think there's an alternative that would be better intergrated into the existing flow.

This component already deals with overlapping load operations and knowing when the final one is finished. So, _loading could be reduced to a bool flag that gets set to true above when we set cancellationToken = _refreshCts.Token;. It can then be set to false inside the block below next to where we set _itemCount = result.TotalItemCount etc, because in that block we know this operation wasn't cancelled and therefore it is the last loading operation within the overlapping group. We don't have to worry about exceptions because there are only two cases:

  1. e is OperationCanceledException oce && oce.CancellationToken == cancellationToken - that means this is not the last loading operation, so we leave the _loading flag unchanged
  2. Any other exception means it's an unhandled exception and the whole component is going to fail and be torn down, so we can also leave the _loading flag unchanged

Would you be OK with trying that approach? I'm asking because a bool flag is more conventional within this system and fits in with how we already know when the final loading process is finished.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely. This is the better approach - thanks!

Added a new commit.

@SteveSandersonMS SteveSandersonMS self-requested a review July 6, 2023 16:52
Co-authored-by: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
@SteveSandersonMS SteveSandersonMS self-requested a review July 6, 2023 20:10
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Looks superb - thanks so much for contributing this!

I'll merge this once the build passes.

@SteveSandersonMS SteveSandersonMS merged commit 15f0daa into dotnet:main Jul 7, 2023
@ghost ghost added this to the 8.0-preview7 milestone Jul 7, 2023
@etemi etemi deleted the issue-28770 branch July 8, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blazor Virtualize: Empty Rows Template
3 participants