MudDataGrid: Configuration using expressions#6049
Conversation
mikes-gh
changed the title
mikes-gh
added
enhancement
|
@tjscience Thanks for the PR. Just a note the root of this branch is way back in time and hence the conflicts. I would advise rebasing this to the head of dev ASAP before doing any more work. |
mikes-gh
changed the title
|
@tjscience I guess you know but this isn't building. |
|
@mikes-gh, it is a rough draft. The whole purpose was to get the foundation out there to be viewed by others. If you want to take a look you should be able to build and run the wasm host project (and obviously every dependency underneath). It is going to take time to go through all of the tests to fix all that this change broke. |
|
@tjscience check the build errors. I cant build MudBlazor :-) |
|
@tjscience: do you have a rough time frame for this PR? I am not pressing you, it is just that as a team we want to plan on how long we are going to have to suppress DataGrid PRs from others. Thanks. |
|
I would like to say end of the month but I have been extremely busy with my day job. I will try and devote some time to it this weekend and next week though. The heavy lifting is done. Primarily, all that is left is fixing all of the tests and putting in more examples. |
|
@tjscience Could you get this building? Not concerned with the tests for now but it would be good to have it building at least so others could test for you. |
|
I am able to build and run the wasm host just fine. Can you give a bit more details about what is failing in the build for you?
|
|
The error is in the PR build logs. |
|
@mikes-gh Try it out now and see if you can build. |
Thanks @tjscience I can build now which is great. |
|
Ignore previous comment. After dotnet clean all ok and builds OK @tjscience 👍 |
|
My team uses MudDataGrid's pretty extensively. Is there a timeline on when this PR might drop? |
Made filtering expressions more efficient by caching the compilation.
We are aware of the problem with TimeOnly / DateOnly and it will be addressed once the major bugs in the DataGrid are fixed.
How does your hacky solution look? |
|
@ScarletKuro as described by @lfhenrl, only simple filtering mode invokes server data reloading, I've also noticed that clearing the filter does not invoke sever data reloading in any mode. I've rushed an initial commit with unfinished tests and found out that there is some kind of a bug with enum filtering too (nullable enum maybe?), anyway the server data is reloaded now, maybe you'd like to add some comments while I finish. |
|
@alfadelta88 doesn't the |
|
@ScarletKuro you're absolutely right, i'll fix it and push it along with the tests in a bit. |
|
@alfadelta88 now when #6614 is merged. We can make much better design for you changes. As all the methods in Example: internal void ClearFilter()
{
...
if (DataGrid.ServerData != null) DataGrid.ReloadServerData().AndForget();
...
}to internal async Task ClearFilterAsync()
{
...
if (DataGrid.ServerData != null) await DataGrid.ReloadServerData();
...
}This will be not a breaking change, because all the methods are internal and only used inside our components and not exposed, all places that call these methods support returning a Task due to this change #6614. @henon FYI |
|
@ScarletKuro great, I'll make necessary changes. |
The issue for me here is that this actually works fine in the previous version (as you could just set the type to DateTime), so it means the next stable release will have removed functionality that can't be replicated if we wait to handle DateOnly across the board instead of supporting it in the filter again. I know it's experimental though, so it's fair game. I was just hoping that, if I had the time/opportunity, I could sneak in a compatible candidate before the next release.
Does the converter work for server data? For some reason, I was under the impression that this was more so for inline data. I was using this suggested workaround, basically (we just changed the inheritance and cascaded extra attributes for better parity): #6178 (comment) |
Yes, if you want to PR DateOnly support for DataGrid date filter please go ahead. You can tag us in your PR for speedy review. If we release before you are done we can also make a pre-release nuget for you once your PR is in. Just make sure you have good test coverage of your changes. |
|
My idea to address this problem is to create an easy abstraction called public interface ITypeFilterMapper
{
Type Type { get; }
RenderFragment RenderFilterFragment<T>(FilterDefinition<T> filterDefinition, Column<T> column);
}This new abstraction can be passed to the DataGrid component. We would define on our side for common mappers for types like String, Boolean, DateTime etc Users can also implement this interface to add support for any custom type they want. foreach (ITypeFilterMapper mapper in TypeFilterMapper)
{
if (filterDefinition.Type == mapper.Type)
{
RenderFilterFragment(filterDefinition, filter, column);
}
}With this approach, users have the flexibility to add any type they want without having to modify the DataGrid component directly. @if (filterDefinition.FieldType.IsDateOnly)
{
<HTML CODE>
}And this is not flexible at all. This is just a high level abstraction that was in my head. |
|
@ScarletKuro I pushed a commit to this branch with the relevant tests, if you could take a look when you have the time. |
|
@ScarletKuro Btw, with |
1 second seems like too much of a delay. I think that 500ms would be better to default to. |
@tjscience You're right, I'll adjust it in the next commit. |
|
I am really looking forward to the release of the MudDataGrid component。 |
|
@alfadelta88 why don't you create a PR? You can push to it even after you created it, and we can comment and discuss. |
|
Yeah, you can make it a draft as well. It will be much easier. |
|
@henon @ScarletKuro sure, here #6627 |
I was out for an extended period of time - how does the team feel about this approach? Is it even favorable (regarding your comment about it not being flexible)? @ScarletKuro have the other major issues with the data grid been resolved such that timeonly/dateonly support is workable? I was looking to upgrade beyond 6.1.9 soon so we can leverage the latest fixes and features for other components. Regardless, I'd like to know the team's opinion on the aforementioned suggestion. |
|
@sjd2021 MudDataGrid has matured, so yes you can work on dateonly and timeonly support without causing conflicts. As for your question, I can't answer it. Maybe @tjscience can. |
|
@henon and @sjd2021 The problem with the @if (filterDefinition.FieldType.IsDateOnly) pattern is that it is becoming unwieldy and it increases complexity each time we need to add a type. I am in favor of @ScarletKuro proposed solution here. This way all of the types can be managed more easily and overridden by the dev. If this is too much work at this point Kuro, we can go ahead and just implement TimeOnly and DateOnly right now and work on the mapper later. |
There is prototype that works #6626, but I still don't fully like the implementation. I will later try to revive work on it. |
|
@ScarletKuro, I do not see this implemented cell's edit rendering. Was that a miss, or just not gotten to yet? |
Not gotten yet, the point was firstly to find an adequate abstraction to dynamically hookup filter parts depending on the type, then everything rest. |
MudDataGrid: Configuration using expressions (MudBlazor#6049) Co-authored-by: Terry Phillips <[email protected]>
henon
added
the
legendary
14 participants
Description
resolves #4567
PR references #5816
This PR is meant to track the changes required to support a property expression as opposed to a string field. The early draft PR is to allow the community and core team to evaluate the progress of this feature as I am developing it out. It is still rough at this point and I have to go through all of the tests, create more and also add better documentation.
I believe that I have fixed up all of the trimming issues as well.
How Has This Been Tested?
Types of changes
Checklist:
dev).