feat(TreeView): make CanExpandWhenDisabled work if set IsDisabled to true #7417
+17
−9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts TreeView disabled and expansion behavior so that CanExpandWhenDisabled works when the TreeView itself is disabled, by centralizing disabled-state logic in TreeViewRow, removing an extension helper, and updating click handling and tests accordingly. Sequence diagram for TreeView click and expand when disabledsequenceDiagram
actor User
participant TreeViewRow as TreeViewRow_TItem_
participant TreeView as TreeView_TItem_
participant TreeViewItem as TreeViewItem_TItem_
User->>TreeViewRow: ClickNode()
TreeViewRow->>TreeViewRow: GetDisabledStatus()
alt disabled and cannot expand
TreeViewRow-->>User: IgnoreClick()
else enabled or can expand when disabled
TreeViewRow->>TreeView: OnClick(TreeViewItem)
TreeView->>TreeView: check ClickToggleNode
alt ClickToggleNode is true
TreeView->>TreeView: OnToggleNodeAsync(TreeViewItem)
else ClickToggleNode is false
TreeView-->>User: NoToggle()
end
end
Class diagram for updated TreeView disabled and expand behaviorclassDiagram
class TreeViewRow_TItem_ {
+bool IsDisabled
+bool CanExpandWhenDisabled
+TreeViewItem_TItem_ Item
+bool GetDisabledStatus()
+bool CanTriggerClickNode
+bool ItemDisabledState
}
class TreeView_TItem_ {
+bool IsDisabled
+bool CanExpandWhenDisabled
+bool ClickToggleNode
+Task OnClick(TreeViewItem_TItem_ item)
+Task OnToggleNodeAsync(TreeViewItem_TItem_ item)
}
class TreeViewItem_TItem_ {
+bool IsDisabled
+bool HasChildren
+bool IsExpand
+List_TreeViewItem_TItem__ Items
}
class TreeViewExtensions {
-bool CanTriggerClickNode_TItem_(TreeViewItem_TItem_ item, bool isDisabled, bool canExpandWhenDisabled)
}
TreeViewExtensions : <<removed>>
TreeViewRow_TItem_ --> TreeViewItem_TItem_ : uses
TreeView_TItem_ --> TreeViewItem_TItem_ : manages
TreeViewRow_TItem_ --> TreeView_TItem_ : parentTreeView
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
TreeView.razor.cs, dropping theitem.CanTriggerClickNode(IsDisabled, CanExpandWhenDisabled)check inOnClickmeans node toggling no longer respects disabled state orCanExpandWhenDisabled; consider reintroducing a guard (e.g., via the newCanTriggerClickNodelogic) so click behavior matches the intended enabled/disabled semantics. - The new
GetDisabledStatusmethod encodes both disabled state andCanExpandWhenDisabledinto a single boolean, which is then negated inCanTriggerClickNode; consider either renaming it to reflect the combined semantics or inlining/clarifying the logic to make it easier to understand how disabled and expansion behavior interact.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TreeView.razor.cs`, dropping the `item.CanTriggerClickNode(IsDisabled, CanExpandWhenDisabled)` check in `OnClick` means node toggling no longer respects disabled state or `CanExpandWhenDisabled`; consider reintroducing a guard (e.g., via the new `CanTriggerClickNode` logic) so click behavior matches the intended enabled/disabled semantics.
- The new `GetDisabledStatus` method encodes both disabled state and `CanExpandWhenDisabled` into a single boolean, which is then negated in `CanTriggerClickNode`; consider either renaming it to reflect the combined semantics or inlining/clarifying the logic to make it easier to understand how disabled and expansion behavior interact.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/TreeView/TreeView.razor.cs:608` </location>
<code_context>
if (confirm)
{
_activeItem = item;
- if (ClickToggleNode && item.CanTriggerClickNode(IsDisabled, CanExpandWhenDisabled))
+ if (ClickToggleNode)
{
await OnToggleNodeAsync(item);
</code_context>
<issue_to_address>
**issue (bug_risk):** Removing the `CanTriggerClickNode` check means disabled trees/items may still toggle on click.
With this change, `OnClick` ignores `IsDisabled`, `Item.IsDisabled`, and `CanExpandWhenDisabled` and only checks `ClickToggleNode`, so disabled nodes (or a disabled tree) can still be expanded/collapsed. If disabled state is meant to prevent interaction, consider keeping a predicate here (e.g., the same one used by `TreeViewRow` for clickability) so disabled nodes don’t respond to clicks.
</issue_to_address>
### Comment 2
<location> `test/UnitTest/Components/TreeViewTest.cs:893` </location>
<code_context>
});
node = cut.Find(".node-icon");
- Assert.Contains("disabled", node.ClassList);
+ Assert.DoesNotContain("disabled", node.ClassList);
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests to cover all combinations of `IsDisabled`, `Item.IsDisabled`, and `CanExpandWhenDisabled` for the icon disabled CSS state.
This now covers `IsDisabled = true` and `CanExpandWhenDisabled = true`, but the new `GetDisabledStatus()` logic has additional important combinations that aren’t exercised:
- `IsDisabled = false`, `Item.IsDisabled = true`, `CanExpandWhenDisabled = true` → icon should **not** be disabled
- `IsDisabled = true`, `Item.IsDisabled = false`, `CanExpandWhenDisabled = false` → icon should be disabled
- `IsDisabled = false`, `Item.IsDisabled = true`, `CanExpandWhenDisabled = false` → icon should be disabled
- All false → icon should not be disabled
Please add tests (e.g., a `[Theory]` with `[InlineData]`) that assert the `disabled` class presence/absence on `.node-icon` for these cases to fully cover `GetDisabledStatus()` and prevent regressions.
Suggested implementation:
```csharp
pb.Add(a => a.IsDisabled, true);
});
node = cut.Find(".node-icon");
Assert.DoesNotContain("disabled", node.ClassList);
}
[Theory]
// All false → icon should not be disabled
[InlineData(false, false, false, false)]
// IsDisabled = true, Item.IsDisabled = false, CanExpandWhenDisabled = false → icon should be disabled
[InlineData(true, false, false, true)]
// IsDisabled = false, Item.IsDisabled = true, CanExpandWhenDisabled = false → icon should be disabled
[InlineData(false, true, false, true)]
// IsDisabled = false, Item.IsDisabled = true, CanExpandWhenDisabled = true → icon should not be disabled
[InlineData(false, true, true, false)]
public void NodeIcon_DisabledClass_Combinations(
bool isDisabled,
bool itemIsDisabled,
bool canExpandWhenDisabled,
bool expectedIconDisabled)
{
// Arrange
using var ctx = new TestContext();
var treeItem = new TreeItem<string>
{
Text = "root",
Value = "root",
IsDisabled = itemIsDisabled,
};
var cut = ctx.RenderComponent<TreeView<string>>(pb =>
{
pb.Add(a => a.Items, new[] { treeItem });
pb.Add(a => a.IsDisabled, isDisabled);
pb.Add(a => a.CanExpandWhenDisabled, canExpandWhenDisabled);
});
// Act
var node = cut.Find(".node-icon");
// Assert
if (expectedIconDisabled)
{
Assert.Contains("disabled", node.ClassList);
}
else
{
Assert.DoesNotContain("disabled", node.ClassList);
}
}
[Fact]
```
If the actual generic parameter type or item type in your `TreeView` tests differs (e.g., you use a custom model instead of `string` or a different `TreeItem<T>` type), adjust `TreeItem<string>` / `TreeView<string>` and the `Text`/`Value` assignments to match the existing tests in `TreeViewTest.cs`. Also ensure `TestContext`, `TreeItem<T>`, and `TreeView<T>` are already imported; if not, add the appropriate `using` statements at the top of the file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (confirm) | ||
| { | ||
| _activeItem = item; | ||
| if (ClickToggleNode && item.CanTriggerClickNode(IsDisabled, CanExpandWhenDisabled)) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Removing the CanTriggerClickNode check means disabled trees/items may still toggle on click.
With this change, OnClick ignores IsDisabled, Item.IsDisabled, and CanExpandWhenDisabled and only checks ClickToggleNode, so disabled nodes (or a disabled tree) can still be expanded/collapsed. If disabled state is meant to prevent interaction, consider keeping a predicate here (e.g., the same one used by TreeViewRow for clickability) so disabled nodes don’t respond to clicks.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7417 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 748 748
Lines 32793 32798 +5
Branches 4551 4551
=========================================
+ Hits 32793 32798 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #7378 by making the CanExpandWhenDisabled property work correctly when the TreeView's IsDisabled property is set to true. Previously, CanExpandWhenDisabled only worked for individually disabled items but was overridden when the entire TreeView was disabled.
- Refactored disabled state logic by introducing a new
GetDisabledStatus()method in TreeViewRow that properly considersCanExpandWhenDisabledfor both tree-level and item-level disabled states - Removed the
CanTriggerClickNodeextension method as its logic has been moved into the TreeViewRow component - Updated test assertions to reflect the new expected behavior where the node-icon should not have the "disabled" class when
CanExpandWhenDisabledis true, even whenIsDisabledis true - Incremented version to 10.1.5-beta02
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTest/Components/TreeViewTest.cs | Updated test assertion to expect node-icon NOT to have "disabled" class when IsDisabled=true and CanExpandWhenDisabled=true |
| src/BootstrapBlazor/Extensions/TreeViewExtensions.cs | Removed the CanTriggerClickNode extension method as its logic was moved to TreeViewRow component |
| src/BootstrapBlazor/Components/TreeView/TreeViewRow.razor.cs | Added GetDisabledStatus() method with refactored logic that properly handles CanExpandWhenDisabled for both tree-level and item-level disabled states |
| src/BootstrapBlazor/Components/TreeView/TreeView.razor.cs | Simplified OnClick method by removing the redundant disabled state check that was previously performed by the extension method |
| src/BootstrapBlazor/BootstrapBlazor.csproj | Version bumped from 10.1.5-beta01 to 10.1.5-beta02 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+608
to
611
| if (ClickToggleNode) | ||
| { | ||
| await OnToggleNodeAsync(item); | ||
| } |
There was a problem hiding this comment.
The removal of the disabled state check in the OnClick method removes a safeguard that was previously in place. While the TreeViewRow.razor file guards the ClickRow call with TriggerClick="!ItemDisabledState", removing the check here means that if OnClick is called programmatically or through other code paths (testing, future features), it will no longer validate whether the node should be clickable based on the disabled state. Consider adding the check back using the new GetDisabledStatus logic or documenting why this check is no longer needed. If the behavior is that clicking the node text should respect ItemDisabledState but not CanExpandWhenDisabled (i.e., text clicking is always disabled when the tree/item is disabled), then this might be acceptable, but it represents a change in the validation strategy.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #7378
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Adjust TreeView node disabled and expansion behavior to honor the CanExpandWhenDisabled setting when items or the whole tree are disabled.
New Features:
Enhancements:
Tests: