-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Document URLs Data Resolver #19316
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
Document URLs Data Resolver #19316
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
This PR updates the Multi URL Picker and document URLs app to support variant-aware URL resolution by introducing and integrating a new DocumentURLsDataResolver. Key changes include:
- Wrapping content in the workspace info app with a new container element for styling.
- Updating the link picker modal and multi URL picker components to use new repository and data resolver methods for fetching document and media URLs.
- Adjusting document URL models and integrating the DocumentURLsDataResolver in the document links workspace info app.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/Umbraco.Web.UI.Client/src/packages/relations/relations/reference/workspace-info-app/entity-references-workspace-view-info.element.ts | Added a container div for layout consistency. |
src/Umbraco.Web.UI.Client/src/packages/multi-url-picker/link-picker-modal/link-picker-modal.element.ts | Replaced legacy document/media repositories with new repository and data resolver usage. |
src/Umbraco.Web.UI.Client/src/packages/multi-url-picker/components/input-multi-url/input-multi-url.element.ts | Integrated asynchronous URL resolution for each link and added resolved URL state. |
src/Umbraco.Web.UI.Client/src/packages/documents/documents/url/repository/types.ts | Updated the culture property from optional to required (accepting null values). |
src/Umbraco.Web.UI.Client/src/packages/documents/documents/url/info-app/document-links-workspace-info-app.element.ts | Adjusted link rendering logic and integrated the DocumentURLsDataResolver. |
src/Umbraco.Web.UI.Client/src/packages/documents/documents/url/index.ts | Exported the new DocumentURLsDataResolver. |
src/Umbraco.Web.UI.Client/src/packages/documents/documents/url/document-urls-data-resolver.ts | Added a new resolver for obtaining variant-aware document URLs. |
src/Umbraco.Web.UI.Client/src/packages/documents/documents/item/document-item-data-resolver.ts | Minor update with a TODO regarding variant context usage. |
src/Umbraco.Web.UI.Client/src/packages/core/workspace/info-app/global-components/workspace-info-app-layout.element.ts | Removed the custom container padding style. |
Comments suppressed due to low confidence (1)
src/Umbraco.Web.UI.Client/src/packages/documents/documents/url/repository/types.ts:7
- [nitpick] Changing the 'culture' property to be non-optional may require verifying that all consumers of UmbDocumentUrlModel supply a defined value (even if null) instead of undefined.
culture: string | null;
...I.Client/src/packages/multi-url-picker/components/input-multi-url/input-multi-url.element.ts
Show resolved
Hide resolved
@AndyButland, you have recently made some UI updates to the Multi URL Picker. Can you help ensure that the feature still works the way it should? |
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.
@madsrasmussen - sure, I've tested it out locally. Mostly seems to work just fine, and I can see the new multi-lingual aware behaviour is functioning nicely, but I found these small things:
- If you select a document, or media, remove it again and switch to the "Manual" tab the previously selected document or media's URL is still displayed. Given I've clicked to remove it and confirmed it via the warning, I think it should be empty at this point.
- Also, having followed the same process, it looks like the "title" isn't reset, as once I've provided a manual URL and closed the picker, the previously selected document or media's title is shown in the document workspace.
- If you clear the title, you get a validation message, but it appears under the anchor tag.
- This may have been my doing with a previous update - I changed the behaviour in this PR so we checked for the existence of a title rather than a URL, as for unpublished pages there is no URL.
- Not sure what's best here - could be that we restore that this isn't mandatory and it's enough to just choose or enter the link (so we need to check for either a unique or a manually entered URL to determine if a link has been correctly populated via the picker).
- Or we keep it mandatory - it is automatically filled for all three modes when a URL is provided - but the validation message should move so it indicates the whole form is invalid rather than one field, and the error message tweaked.
src/Umbraco.Web.UI.Client/src/packages/documents/documents/url/document-urls-data-resolver.ts
Show resolved
Hide resolved
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.
Good, but we need to update with Variant Context
Fixes #18317
This PR updates the Multi URL Picker UI to render variant-aware URLs and adds the variant name to the URL title field. It introduces a DocumentURLsDataResolver to obtain the variant-aware URLs. This resolver is now reused for both the workspace URLs in the info app and the Multi URL Picker.
The workspace URLs now display only the URLs for the active variant unless the document is invariant; in that case, it will show all URLs.
Updates to the Multi URL Picker
https://github.com/user-attachments/assets/f6f8a7b7-f747-4aad-8250-c2edfeb175ac
Updates to the document URLs app
https://github.com/user-attachments/assets/f3fc25ee-8ee4-42e8-8459-a6272dcd67fd