-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Do not set icon color if the item is selected #19404
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for treating the selected state like the active state when determining icon coloration in tree items.
- Include
_isSelected
in the conditional that picks the “iconWithoutColor” variant for active and selected items. - Expose
_isSelected
as aprotected
property so subclasses can reference it. - Update both document and media tree item components, plus the base tree-item renderer.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/packages/media/media/tree/tree-item/media-tree-item.element.ts | Updated icon to use iconWithoutColor when the item is selected |
src/packages/documents/documents/tree/tree-item/document-tree-item.element.ts | Updated icon to use iconWithoutColor when the item is selected |
src/packages/core/tree/tree-item/tree-item-base/tree-item-element-base.ts | Changed _isSelected to protected and added selected-state check to default icon rendering |
Comments suppressed due to low confidence (2)
src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-base/tree-item-element-base.ts:74
- [nitpick] Exposing
_isSelected
as a protected field breaks encapsulation. Consider keeping it private and providing a protected or public getterisSelected()
for subclasses to use.
protected _isSelected = false;
src/Umbraco.Web.UI.Client/src/packages/media/media/tree/tree-item/media-tree-item.element.ts:15
- There’s new behavior for the selected state affecting icon rendering. Add unit tests to ensure
iconWithoutColor
is used when_isSelected
is true.
<span id="icon-container" slot="icon">
@@ -14,7 +14,7 @@ export class UmbMediaTreeItemElement extends UmbTreeItemElementBase<UmbMediaTree | |||
<span id="icon-container" slot="icon"> | |||
${icon && iconWithoutColor | |||
? html` | |||
<umb-icon id="icon" slot="icon" name="${this._isActive ? iconWithoutColor : icon}"></umb-icon> | |||
<umb-icon id="icon" slot="icon" name="${this._isActive || this._isSelected ? iconWithoutColor : icon}"></umb-icon> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same icon color logic is duplicated in multiple components. Consider extracting this conditional into a shared helper or base render method to avoid drift and keep behavior consistent.
Copilot uses AI. Check for mistakes.
@@ -71,7 +71,7 @@ export abstract class UmbTreeItemElementBase< | |||
private _isSelectable = false; | |||
|
|||
@state() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a JSDoc comment explaining why _isSelected
is now protected and how it impacts icon rendering to improve code readability for future maintainers.
@state() | |
@state() | |
/** | |
* Indicates whether the tree item is selected. | |
* | |
* This property is marked as `protected` to allow subclasses to customize | |
* the behavior of selection state handling while preventing external access. | |
* The `_isSelected` state directly impacts the rendering of the item's icon, | |
* ensuring that the correct visual representation is displayed when the item | |
* is selected. | |
*/ |
Copilot uses AI. Check for mistakes.
If there's an existing issue for this PR then this fixes #19400
This is an addition to PR #17703, so the same behavior happens for the selected state.
This can be tested using the document picker, multi url picker or by selecting "Allowed child node types" in document or media types.