Megabit
DataGrid: Fixes Toggleable for DetailRow
Description
Closes #6049
- Checks Toggleable first
- Simplifies
SetRowDetail - Clarify comments
The test doesn't pass, because it goes against the idea of Toggleable.
https://github.com/Megabit/Blazorise/blob/f6d75dcb1c3932b697319d960cdd6e31b0340762/Tests/Blazorise.Tests/Components/DataGridDetailRowComponentTest.cs#L105
I am not sure what was the intention for Toggleable, but I would suspect it should prevents toggling when set to false, but the test implies otherwise.
Is there something behind such behavior @stsrki ?
With Toggleable being ignored under some conditions it's impossible to fix the bug.
The test doesn't pass, because it goes against the idea of
Toggleable.https://github.com/Megabit/Blazorise/blob/f6d75dcb1c3932b697319d960cdd6e31b0340762/Tests/Blazorise.Tests/Components/DataGridDetailRowComponentTest.cs#L105
I am not sure what was the intention for
Toggleable, but I would suspect it should prevents toggling when set tofalse, but the test implies otherwise.Is there something behind such behavior @stsrki ?
With
Toggleablebeing ignored under some conditions it's impossible to fix the bug.
Based on a name, yeah, it should not be able to toggle it. But I was not the original creator, so I'm not sure.
@David-Moreira any chance you could remember this?
The test doesn't pass, because it goes against the idea of
Toggleable. https://github.com/Megabit/Blazorise/blob/f6d75dcb1c3932b697319d960cdd6e31b0340762/Tests/Blazorise.Tests/Components/DataGridDetailRowComponentTest.cs#L105I am not sure what was the intention for
Toggleable, but I would suspect it should prevents toggling when set tofalse, but the test implies otherwise. Is there something behind such behavior @stsrki ? WithToggleablebeing ignored under some conditions it's impossible to fix the bug.Based on a name, yeah, it should not be able to toggle it. But I was not the original creator, so I'm not sure.
@David-Moreira any chance you could remember this?
Oh man, having to remember things developed centuries ago!! If I remember correctly toggle as the name implies, makes it so the DetailRow can have a toggle mechanism regardless of the evaluated function for RowDetail in subsequent evaluations.
I.e
Row 1 evaluates that it should show rowdetail => FUNC returns true
As a user if toggleable is set to true, then I can click on it and even though FUNC still returns true, it will be set to false and hide the Row Detail instead just like a toggle.
Practical example : Let's say that you show Detail Rows for every customer that has ages over 30. The func will evaluate and display every detail row.
-
If toggleable is set to false, whenever you try to click on a row with customer age > 30
- It should stay with detail if im not mistaken
-
If toggleable is set to true, whenever you try to click on a row with customer age > 30
- it will collapse the detail as per the toggle
-
If toggleable is set to false, whenever you try you try to click on a row with customer age < 30
- Nothing happens
-
If toggleable is set to true, whenever you try to click on a row with customer age < 30
- Even though the condition is not verified I believe it will show the detail as per the toggle
So as you can see, by enabling or disabling the flag you can have slight variations of the behaviour and that's why it exists and also why it's important to have nice examples of these variations in the docs, because the more flags you have the harder it is to understand the different outcomes. Also the thing with evolving features, trying to keep old behaviours in, while allowing new behaviours...
The behaviour I described is what I remember and might not be working exactly as I described, but I believe it should.
https://github.com/Megabit/Blazorise/pull/3008 - PR that introduces new mechanism for handling DetailRow https://github.com/Megabit/Blazorise/pull/3330 - PR that seems to introduce the Toggleable feature
Edit: Might be useful to read the release notes of these PRs, since the PRs themselves don't have alot of information unfortunately.
PS: I was just looking briefly at the code of this PR, it seems that we'll might be loosing the behaviour that I explained and this will be a breaking change, but I'm not sure please check
The new code seems to only call the SetRowDetail function ONLY if Toggeable is set to true, where before it would always call it, so I'm not 100% this logic is equivalent to the one before.
Although I like the idea of the change to simplify SetRowDetail. It's of note that this is a public api and I beleive it's also a breaking change for whoever that might be using that "special" logic.
Edit2: On a separate note regarding conventions I don't think we use new suffixes to represent arguments that mutate state.
The method call already implies the change/state mutation. and "new" suffix seems redundant and not something we do in the code base I think so I would kindly suggest this version:
public void SetRowDetail( bool hasDetailRow )
If we were to be explicit about it mutating no matter what, I think we've used "force" prefix before? Anyway that's up to you guys, just a kind suggestion regarding that particular suffix.
Yes, thank you for the intervention.
If toggleable is set to false, whenever you try to click on a row with customer age > 30
- It should stay with detail if im not mistaken
That's actually true and it answers the original question: "How to prevent the closing of the DetailRow".
Who would have thought that the answer would be "by returning true in DetailRowTrigger".
Which means this PR is no longer needed—at least not for fixing the bug.
What I don't like is the absolutely overengineered solution where one state is controlled in three different ways that interact with each other in unpredictable ways (unless you are the author of the code).
-
Toggleablemeans:Toggleable_Sometimes_When_Some_Other_Conditions_Are_Met_And_Can_Be_Overridden_By_Force_Toggle -
DetailRowTriggermeans:Yeah_I_Might_Open_The_Detail_Row_But_It_Dependz_Go_Read_Docs_Good_Luck_Lol
The only one that makes sense is the forced toggle—it does exactly what the name/argument suggests.
I understand the overengineering came from stacking up features over time.
It would require some breaking changes, but I can imagine a simple solution where variable naming makes sense and actually does what it’s supposed to do.
I feel like I know the answer, but I’ll ask anyway: @stsrki Is it something worth fixing?
Glad I could help.
What I don't like is the absolutely overengineered solution where one state is controlled in three different ways that interact with each other in unpredictable ways (unless you are the author of the code).
While I am not the original author of the feature, I am indeed the author of the refactoring of this feature and the adaptations that came next. I am totally fine with critics. Maybe I could have come up with something better at the time, I don't know, that's what I came up with at the time with my knowledge/skills to replace what we had. Which, if I remember correctly was triggering on every state change. So we had to come up with something while trying to not do any breaking behavioral changes for users and also building new features on top of it as time went by.
If you can come with a simpler solution that is clearer and complies with every feature without breaking things for users..., hey, it's always worth it in my opinion!! Of course, you and @stsrki are the one's that have to decide that. :)
Again, glad I was helpful, just let me know if you need anything.
Hm, that might have sounded like I was "bashing" you for knowing "what’s actually going on." It's quite the opposite — I'm glad you shared your knowledge and took the time to explain it to us.
If you can come with a simpler solution that is clearer and complies with every feature without breaking things for users
Ha ha - No, I don't think so. It would have to be a breaking change.
Converting to draft until we figure out what to do here. If it's a breaking change or behavior, then we will do it in the next release. If at all.