-
Notifications
You must be signed in to change notification settings - Fork 2.8k
propertyValuePresentation extension type for displaying complex property values (fixes #20736)
#20748
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
…nly and time only pickers.
… in context constructor and filtering from that.
nielslyngsoe
left a comment
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.
I had a look at the code a gave a few comments and perspectives — looks good overall, but there is a few thoughts to make it simpler and to avoid perserving specific names for the system fields.
...aco.Web.UI.Client/src/packages/documents/documents/collection/document-collection.context.ts
Outdated
Show resolved
Hide resolved
src/Umbraco.Web.UI.Client/src/packages/property-editors/color-picker/manifests.ts
Outdated
Show resolved
Hide resolved
....UI.Client/src/packages/core/property-value-presentation/property-value-presentation-base.ts
Outdated
Show resolved
Hide resolved
...aco.Web.UI.Client/src/packages/documents/documents/collection/document-collection.context.ts
Outdated
Show resolved
Hide resolved
...aco.Web.UI.Client/src/packages/documents/documents/collection/document-collection.context.ts
Outdated
Show resolved
Hide resolved
...lient/src/packages/property-editors/color-picker/property-value-presentation-color-picker.ts
Outdated
Show resolved
Hide resolved
…r property values (not system values)
as we shouldn't really be generating markup from here. This brings some duplication back between the grid/table collection-views, we can refine in due course.
|
Thanks for all the updates and improvements @leekelleher - I pulled it down this morning for another look, and all works as expected with better performance. Firstly, just a question so I understand moving forward - previously I had some code to see if a property value extension exists for a given property editor alias, and if so, used So am I right in understanding that if an extension isn't found for that type and filer, the default content for that element - i.e. Even doing that though I do see a small slowness in rendering. Maybe it's not a big deal, but for example if you add to a collection view one property that has a property presentation value extension (e.g. a colour picker) and one that doesn't (e.g. a text field), the latter will appear quicker. Not really sure why that should be the case given both seem to go through the same code path apart from the property presentation value extension itself. |
Yes, that's correct. Following @nielslyngsoe's comment: #20748 (comment), the slotted content will render if there are no extensions loaded/rendered.
I've noticed this too, especially with a Grid collection-view, toggling a card's select/deselect, there's a lag in the re-render. |
Yes, it's more obvious here. It looks like you also see briefly the slotted content. Looking closely I can see the "[object Object]" string rendering before it's replaced by the result of the property value presentation. |
…e system/property value lookups
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
This PR introduces a new extension point (propertyValuePresentation) for property editors to customize how complex property values are displayed in collection views (tables and grids). Previously, complex objects stored as JSON displayed as [object Object]. The PR implements this new extension for five date/time pickers and the color picker, providing formatted output for dates and visual swatches for colors.
Key Changes:
- Created new
propertyValuePresentationextension type with base element - Implemented property value presentation components for date pickers (date-only, time-only, datetime, datetime-with-timezone) and color picker
- Refactored document and media collection views to use the new extension system
- Added
editorAliasto collection item value models to enable property editor lookup - Introduced
UmbDocumentCollectionItemDataResolverto centralize collection item data resolution logic
Reviewed Changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Added import alias for new property-value-presentation package |
| package.json | Exported new property-value-presentation package |
| vite.config.ts | Added build configuration for property-value-presentation |
| property-value-presentation/* | Created new extension type, base element, and type definitions |
| -picker/property-value-presentation-.element.ts | Implemented presentation elements for 5 date pickers and color picker |
| *-picker/manifests.ts | Registered property value presentation manifests for each picker |
| document-collection-item-data-resolver.ts | New resolver extending UmbDocumentItemDataResolver for collection-specific logic |
| document-item-data-resolver.ts | Changed #getCurrentVariant to protected _getCurrentVariant for inheritance |
| document-table-column-*.element.ts | Split into system-value and property-value column components |
| document-grid-collection-card.element.ts | Updated to use resolver and extension slot for property rendering |
| media-table-column-*.element.ts | Created system-value and property-value column components for media |
| types.ts | Added editorAlias field to collection item value models |
| *-collection.server.data-source.ts | Mapped editorAlias from server response |
propertyValuePresentation extension type for displaying complex property values (fixes #20736)
|
@AndyButland I have made the de-duplication change (for the system value lookup), but I haven't made any progress with the loading/lag of slotted content. I'm hoping @nielslyngsoe and @iOvergaard may have insights on this. I think the PR is in a good state to merge in and we could pick up on the performance issues in a follow-up PR. |
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
Copilot reviewed 34 out of 34 changed files in this pull request and generated 5 comments.
.../packages/documents/documents/collection/views/grid/document-grid-collection-card.element.ts
Outdated
Show resolved
Hide resolved
...media/media/collection/views/table/column-layouts/media-table-column-system-value.element.ts
Show resolved
Hide resolved
| const date = new Date(this.value.date).toLocaleDateString(); | ||
| return date ? html`<span>${date}</span>` : nothing; |
Copilot
AI
Nov 12, 2025
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 check date ? is redundant because toLocaleDateString() always returns a string. Additionally, if this.value.date is an invalid date string, new Date(this.value.date).toLocaleDateString() will return "Invalid Date", which would be displayed to the user. Consider validating the date and returning nothing if invalid:
const dateObj = new Date(this.value.date);
if (isNaN(dateObj.getTime())) return nothing;
const date = dateObj.toLocaleDateString();
return html`<span>${date}</span>`;| const date = new Date(this.value.date).toLocaleDateString(); | |
| return date ? html`<span>${date}</span>` : nothing; | |
| const dateObj = new Date(this.value.date); | |
| if (isNaN(dateObj.getTime())) return nothing; | |
| const date = dateObj.toLocaleDateString(); | |
| return html`<span>${date}</span>`; |
| const date = new Date(this.value.date).toLocaleString(); | ||
| return date ? html`<span>${date}</span>` : nothing; |
Copilot
AI
Nov 12, 2025
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 check date ? is redundant because toLocaleString() always returns a string. Additionally, if this.value.date is an invalid date string, new Date(this.value.date).toLocaleString() will return "Invalid Date", which would be displayed to the user. Consider validating the date and returning nothing if invalid:
const dateObj = new Date(this.value.date);
if (isNaN(dateObj.getTime())) return nothing;
const date = dateObj.toLocaleString();
return html`<span>${date}</span>`;| const date = new Date(this.value.date).toLocaleString(); | |
| return date ? html`<span>${date}</span>` : nothing; | |
| const dateObj = new Date(this.value.date); | |
| if (isNaN(dateObj.getTime())) return nothing; | |
| const date = dateObj.toLocaleString(); | |
| return html`<span>${date}</span>`; |
| #renderDate() { | ||
| if (!this.value?.date) return nothing; | ||
| const date = new Date(this.value.date).toLocaleString(); | ||
| return html`${date}`; | ||
| } |
Copilot
AI
Nov 12, 2025
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.
If this.value.date is an invalid date string, new Date(this.value.date).toLocaleString() will return "Invalid Date", which would be displayed to the user. Consider validating the date and returning nothing if invalid:
#renderDate() {
if (!this.value?.date) return nothing;
const dateObj = new Date(this.value.date);
if (isNaN(dateObj.getTime())) return nothing;
return html`${dateObj.toLocaleString()}`;
}…lection/views/grid/document-grid-collection-card.element.ts Co-authored-by: Copilot <[email protected]>
Looking into the umb-extension-slot element, it seems it follows a cycle of the following: Frame 1: _permitted is undefined → renders nothing (blank) This is why you see "[object Object]" briefly - it's the slotted fallback content showing during async loading. I think we can solve it by caching the extensions somehow in the extension slot, but I am not quite sure about how to achieve that. Might need some higher-up refactoring in the extensions registry...?
I agree with @leekelleher's sentiment here to merge as-is (after fixing the Copilot suggestions), and then follow up on the performance concerns later on: |
|
An additional performance improvement we should consider: I just had a quick look at the PR, and it appears that we are rendering an extension slot for every cell. I believe we can safely assume that all cells within a column will have the same editorAlias and can, therefore, use the same extension result. If we initialize the extension programmatically by using one of the extension initializers instead of the extension-slot, we will have only one subscriber per column instead of one per cell. We might have to think about this across all collection views somehow. Maybe it needs to be handled in the Collection Context so all views can share the same extension result for an editorAlias. |
Prerequisites
Fixes #20736
Description
This is a WIP PR looking to provide an extension point for property editors to improve presentation in collections. Currently complex objects that are stored as JSON - including the new date pickers - just appears as
[object Object].Here I'm looking to add a new extension point that each property editor can optionally provide to give a presentation, customizable for the different views in which they are displayed.
Testing
Add any or all of the following properties to a collection view:
TODO:
Use the correct import map alias instead of a relative import paththat I can't see how to resolve.umb-extension-slotpresumably still requires a filter through all extension, even though I've isolated the one to use.