Fix "allow children" property on Custom Item Types#108
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements support for the allowChildren property on custom item types in the UmbNav navigation editor. The feature allows developers to create custom item types (like dividers or buttons) that cannot have child items, while preserving the default behavior of allowing children for built-in item types.
Changes:
- Added
allowChildrenproperty to theModelEntryTypetype definition - Added
allowChildrenproperty and logic to theumbnav-itemcomponent to conditionally hide the expand/collapse arrow - Updated
umbnav-groupcomponent to respect theallowChildrenproperty when creating custom items and rendering the UI
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Umbraco.Community.UmbNav/src/tokens/umbnav.token.ts | Adds the optional allowChildren property to the ModelEntryType type definition |
| Umbraco.Community.UmbNav/src/components/umbnav-item/umbnav-item.ts | Adds allowChildren property with default value true and hides expand arrow when set to false |
| Umbraco.Community.UmbNav/src/components/umbnav-group/umbnav-group.ts | Sets allowChildren from registration, prevents expansion and applies collapsed class for items that don't allow children |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| noreferrer: defaults.noreferrer ?? null, | ||
| image: defaults.image ?? [], | ||
| children: [], | ||
| allowChildren: registration.allowChildren |
There was a problem hiding this comment.
The allowChildren property is set directly from registration.allowChildren without a default value. While the current logic correctly treats undefined as allowing children (via the !== false checks), it would be more explicit and maintainable to set a default value here. Consider using allowChildren: registration.allowChildren ?? true to make the default behavior more obvious and consistent with the component's default value.
| allowChildren: registration.allowChildren | |
| allowChildren: registration.allowChildren ?? true |
| noreferrer: defaults.noreferrer ?? null, | ||
| image: defaults.image ?? [], | ||
| children: [], | ||
| allowChildren: registration.allowChildren | ||
| }; | ||
| this.#addItem(newItem); | ||
| } |
There was a problem hiding this comment.
Consider adding test coverage to verify that items created with allowChildren: false do not display the expand/collapse arrow and do not allow children to be added. The current tests verify that custom items can be created with allowChildren: false, but they don't verify the functional behavior of this property.
AllowChildren is now respected when creating custom items.
Default behavior (allowChildren: true) is preserved for built-in items.