-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Support querystring and anchor for local links in Delivery API output #20142
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
Conversation
|
@kjac an option may also be to split the hash into a But currently both exist in the Querystring field in link picker, which is fine as long fragment is the last part (but I don't think there are specific validation for this at the moment). |
AndyButland
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.
Looks good from my perspective, apart from a small issue that needs resolving to avoid a binary breaking change (noted inline). Fine for me to merge once that is resolved, and when you have considered @bjarnef's feedback too.
Have tested with various links, with expected results:
As Markup:
<p>Test <a href=\"/test-landing/#test\" title=\"Test Landing\" data-anchor=\"#test\" data-start-item-path=\"home\" data-start-item-id=\"ac2038d9-dfc2-4294-b778-7edf90d1a178\">link</a>.</p>\n
<p>Test <a href=\"/?foo=bar\" title=\"Home\" data-anchor=\"?foo=bar\" data-start-item-path=\"home\" data-start-item-id=\"ac2038d9-dfc2-4294-b778-7edf90d1a178\">link 2</a>.</p>\n
<p>Test <a href=\"https://umbraco.com?foo=bar\" data-anchor=\"?foo=bar\">link 3</a>.</p>
As JSON:
"elements": [
{
"text": "Test ",
"tag": "#text"
},
{
"tag": "a",
"attributes": {
"anchor": "#test",
"title": "Test Landing",
"route": {
"path": "/test-landing/",
"queryString": "#test",
"startItem": {
"id": "ac2038d9-dfc2-4294-b778-7edf90d1a178",
"path": "home"
}
}
},
"elements": [
{
"text": "link",
"tag": "#text"
}
]
},
{
"text": ".",
"tag": "#text"
}
]
},
{
"tag": "p",
"attributes": {},
"elements": [
{
"text": "Test ",
"tag": "#text"
},
{
"tag": "a",
"attributes": {
"anchor": "?foo=bar",
"title": "Home",
"route": {
"path": "/",
"queryString": "?foo=bar",
"startItem": {
"id": "ac2038d9-dfc2-4294-b778-7edf90d1a178",
"path": "home"
}
}
},
"elements": [
{
"text": "link 2",
"tag": "#text"
}
]
},
{
"text": ".",
"tag": "#text"
}
]
},
{
"tag": "p",
"attributes": {},
"elements": [
{
"text": "Test ",
"tag": "#text"
},
{
"tag": "a",
"attributes": {
"href": "https://umbraco.com?foo=bar",
"anchor": "?foo=bar"
},
"elements": [
{
"text": "link 3",
"tag": "#text"
}
]
},
{
"text": ".",
"tag": "#text"
}
]
}
],
|
Yes, technically hash isn't part of |
…#20142) * Support querystring and anchor for local links in Delivery API output * Add default implementation for backwards compat * Add default implementation for backwards compat (also on the interface) * Fix default implementation
* Support querystring and anchor for local links in Delivery API output (#20142) * Support querystring and anchor for local links in Delivery API output * Add default implementation for backwards compat * Add default implementation for backwards compat (also on the interface) * Fix default implementation * Add extra tests proving that querystring/postfix can be handled for local links in both legacy and current format.

Prerequisites
If there's an existing issue for this PR then this fixes #18475
Description
See the linked issue for a description. This PR fixes the issue.
As it turns out, this:
Property naming
Although this fix applies to both querystrings and anchors, I have opted to call the new property
QueryStringto align with theApiLinkused by the multi URL picker to generate output for the Delivery API. ForApiLink, theQueryStringproperty can also contain anchors.Testing this PR
Make sure that both querystrings and anchor links are included for:
Remember to test with: