8000 Pages MetaData Editing Mode Support by robearlam · Pull Request #29 · Sitecore/ASP.NET-Core-SDK · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Pages MetaData Editing Mode Support #29

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 38 commits into from
May 27, 2025
Merged

Pages MetaData Editing Mode Support #29

merged 38 commits into from
May 27, 2025

Conversation

robearlam
Copy link
Member

This PR adds support for Pages MetaData Editing.

Description / Motivation

  • Introduction of a new Library to be referenced in XMC projects
  • Changes required in LayoutService.Client project to introduce concept of Field Chromes, which weren't required previously.
  • Tests updated to reflect new and updated code

Testing

  • The Unit & Intergration tests are passing.
  • I have added the necessary tests to cover my changes.

Terms

Closes #17

@robearlam
MetaData Config & Render calls configured
bb946a5
@robearlam
Configured metadata render method to correctly render page using new …
3c023f6 
…EditingQuery
@robearlam
Moved GQL logic out of LC.CLient and back into Pages logic. Introduce…
5a1d70d 
…d EditScripts TagHelper
@robearlam
Wired up Placeholder and Rendering chromes
1c8752e
@robearlam
Metadata editing now working
bd0dcde
@robearlam
Wired up EditingDictionaryQuery requests
2b99fc6
@robearlam
Fixed failing unit tests
5f34722
@robearlam
Added UnitTests for EditingScriptsTagHelper
5494e8e
@robearlam
Added UnitTests for Pages GQL Factory, and RenderingEngine TagHelpers
d007e5b
@robearlam
ComponentName is now persisted in rendering collection, to be returne…
b92f31d 
…d in editing config request. Updated related tests
@robearlam
Added UnitTests for Pages Config Middleware
01a9228
@robearlam
Added Unit Tests for Pages Setup Render Middleware call
5be1780
@robearlam
Added UniTests for GraphQLEditingServiceHandler
4ea37ae
@robearlam
Added UniTests for EditingChromes being added to response for Pages r…
ddb0716 
…endering
@robearlam robearlam added the enhancement New feature or request label Feb 17, 2025
@robearlam robearlam self-assigned this Feb 17, 2025
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 14, 2025
View reviewed changes
jballe
jballe reviewed Mar 31, 2025
View reviewed changes
@jballe
Copy link
Contributor
jballe commented Mar 31, 2025

I have tried this and it is working (almost) as expected 👍

When I have the SXA LinkList rendering inserted on a page then editing that page in Pages is failing:
image

I think the reason is that this rendering has an Integrated GraphQL query specified in Sitecore.
This is rendering in the layout response as an object, similar like the standard fields.

Layout response with ContentResolvers are properly handled, because it is rendered as an array. When parsing the layout in

ASP.NET-Core-SDK/src/Sitecore.AspNetCore.SDK.LayoutService.Client/Serialization/Converter/FieldParser.cs

Line 22 in af13261

case JsonTokenType.StartObject:
and using the CustomContrentFieldKey and hereby handled in

ASP.NET-Core-SDK/src/Sitecore.AspNetCore.SDK.Pages/Request/Handlers/GraphQL/GraphQLEditingServiceHandler.cs

Line 195 in af13261

if (field.Value is JsonSerializedField serialisedField && field.Key != "CustomContent")

However this result from the Integrated Query is not going into this code path but is ending up as a standard field with the serialization error shown.
The LinkList is working with ordinary rendering.

I completely acknowledge that this is something we need to handle in the ASP.NET SDK because we are using model binding with (real) strong types while the node based SDK is (more or less) just ignoring this difference 😄

@robearlam
Copy link
Member Author
robearlam commented Apr 1, 2025 •
edited
Loading

I have tried this and it is working (almost) as expected 👍

When I have the SXA LinkList rendering inserted on a page then editing that page in Pages is failing: image

I think the reason is that this rendering has an Integrated GraphQL query specified in Sitecore. This is rendering in the layout response as an object, similar like the standard fields.

Layout response with ContentResolvers are properly handled, because it is rendered as an array. When parsing the layout in

ASP.NET-Core-SDK/src/Sitecore.AspNetCore.SDK.LayoutService.Client/Serialization/Converter/FieldParser.cs

Line 22 in af13261

case JsonTokenType.StartObject:

and using the CustomContrentFieldKey and hereby handled in

ASP.NET-Core-SDK/src/Sitecore.AspNetCore.SDK.Pages/Request/Handlers/GraphQL/GraphQLEditingServiceHandler.cs

Line 195 in af13261

if (field.Value is JsonSerializedField serialisedField && field.Key != "CustomContent")

However this result from the Integrated Query is not going into this code path but is ending up as a standard field with the serialization error shown. The LinkList is working with ordinary rendering.

I completely acknowledge that this is something we need to handle in the ASP.NET SDK because we are using model binding with (real) strong types while the node based SDK is (more or less) just ignoring this difference 😄

Good catch @jballe, I'd forgotten about the iGQL in use on that control.

I've updated the error handling in the GraphQLEditingServiceHandler so that this doesn't blow up the page. The component now renders on the page and is draggable within the placeholders, however while it's fields are rendered correctly they're not inline editable.

I think this will work for now, and we can say that anything implemented with a custom Rendering Contents Resolvers, or with iGQL that changes the structure of the data returned will no longer be editable.

We can then have a look at how to better handle deserialisation of these custom types, as the strong typing model in dotnet will make this more challenging than in node as you mentioned!

@jballe
Copy link
Contributor
jballe commented Apr 6, 2025

Thank you @robearlam that was really fast! I have tried it and it is now working with those changes.

I have two other comments - with the risk of delaying the PR (and especially when I have been poking @IvanLieckens it might not be the best timing) - so let me just give the input and then you can decide if it should actually be part of this PR or a future improvement - or maybe even something unrelated to this...

When using metadata editing in Pages there is a request to config endpoint, implemented in the PagesSetupController. I had to make two changes here to get rid of console/CORS errors, so I have added [HttpOptions] and I have added httpResponse.Headers.AccessControlAllowHeaders = "Authorization";. I don't see the need for the Authorization header but Pages include it in the request (at least currently) so without the request is blocked.
However, it might as well "just" be incorrect behavior in Pages, I also see this CORS error in a newly created JSS based Play Summit demo instance...

More important:
The config endpoint returns a list of (supported) components.
I can see that the PagesSetupController returns the ComponentName value from ComponentRendererDescriptor instances in renderingEngineOptions.RendererRegistry.
However, the values here are not the component name value used to match with the value from the rendering item in Sitecore (that value is compiled to a Predicate and stored in the Match property).
When using the AddModelBoundView extension method, the value stored in ComponentName is actually the view name. So with this implementation you cannot edit components with a view name different than the field in Sitecore, even though the extension method has an overload to specify this.

In this example I have moved the Promo view to a subfolder and changed the view name with a registration like this:

renderingEngineOptions.AddModelBoundView<Promo>("Promo", "HeadlessExperienceAccelerator/Promo")

image
There is not really any indication in the UI, why the rendering is "disabled" but the rendered source code give a clue
image
The response from the config endpoint is:
image

For ViewComponents it is actually worse, the extension method to register the view component

ASP.NET-Core-SDK/src/Sitecore.AspNetCore.SDK.RenderingEngine/Extensions/RenderingEngineOptionsExtensions.cs

Line 154 in 17e0b09

ComponentRendererDescriptor descriptor = ViewComponentComponentRenderer.Describe(match, viewComponentName);

But the method signature is changed to have a locator as a second paramter, so we end up with a registration with an empty componentName

ASP.NET-Core-SDK/src/Sitecore.AspNetCore.SDK.RenderingEngine/Rendering/ViewComponentComponentRenderer.cs

Line 34 in 17e0b09

public static ComponentRendererDescriptor Describe(Predicate<string> match, string locator, string componentName = "")

I can see you have had a bit back and forth on how this actually should be so it was probably not changed everywhere.
I also notice that there is a commit "Changed to no longer persist component names for predicate registrations as its not supported by MetaData Editing" 2 weeks ago so there might be details I don't see (and I don't get what's not supported)

But, from what I see, I think we need to store the actual component name that is used to build the predicate so that it can be exposed in the config endpoint. Right now I think there is a bit confusion on what is the component name used in the layout response and what is the name used to render the component.

@robearlam
Copy link
Member Author

Thanks again for the feedback @jballe. Chatting with Ivan, we're hoping to get this completed and merged in the next couple of weeks.

With the points you raised above:

  • Config OPTIONS call - Fixed!
  • Typed ModelBoundView registration - Fixed!

I also noticed we weren't handling the naming for PartialView registration either so added that in as well.

You're right, we have gone back and forth a bit having discussions about what will be supported in this Component list. The main thing that won't be is the predicate matching e.g. .AddModelBoundView<BoundGenericBlock>(name => name.StartsWith("sc"), "GenericBlock"), as there is no way to return all of the Components matching the predicate for use in the list.

After discussing, we didn't want to hold up the release of this feature and figured as predicate matching isn't a feature offered by JSS, we would move without Pages MetaData supporting it for now and we can figure that out after the fact.

@jballe
Copy link
Contributor
jballe commented Apr 7, 2025

So fast again. We are really looking forward to this, thank you.

I agree, it looks tempting with such generic block and registration (which is then a new feature in the ASP.NET SDK, even though the default view is using that internally) but then you cannot expect that to work in Pages. When making the "usual" registrations it should work in Pages out of the box (and I see the matching more as an implementation detail on the inside)

@Amir818
Copy link
Amir818 commented Apr 22, 2025 •
edited
Loading

@robearlam, there seems to be an issue in this PR where the placeholders are not rendering properly. After lots of digging around, it seems that Pages is not properly outputting the needed HTML DOM elements. Compared to the chrome mode, it seems that the elements and attributes needed inside the <div class="sc-empty-placeholder"> node are not present. Prominently, the <div id="emptySpacerContainer" ... > is missing, and the <code> nodes are missing many of the attributes.

@robearlam
A3E2
Copy link
Member Author

@robearlam, there seems to be an issue in this PR where the placeholders are not rendering properly. After lots of digging around, it seems that Pages is not properly outputting the needed HTML DOM elements. Compared to the chrome mode, it seems that the elements and attributes needed inside the <div class="sc-empty-placeholder"> node are not present. Prominently, the <div id="emptySpacerContainer" ... > is missing, and the <code> nodes are missing many of the attributes.

Good spot Amir. Turns out I was using the incorrect placeholder ID for nested placholder code blocks, that was causing them to render incorrectly, when empty. Should be fixed now.

@jballe
Copy link
Contributor
jballe commented May 22, 2025

Please be aware that ExperienceEditor will still need to be enabled for Preview to work in Pages (and taking Thumbnails in Content Editor but I am in doubt if that is really supported)

@robearlam
Copy link
Member Author

Please be aware that ExperienceEditor will still need to be enabled for Preview to work in Pages (and taking Thumbnails in Content Editor but I am in doubt if that is really supported)

Yeah that is true. This is the same situation with the JS based ContentSDK, and there is work being performed internally to shift Preview away from using the EE.

We can still merge this in for now, and once that work is complete then add the functionality in to the SDK here at that point.

sc-ivanlieckens
sc-ivanlieckens requested changes May 26, 2025
View reviewed changes
@IvanLieckens
Style refactor
0bc23b7 
+ QL vs Ql fixed (added QL to abbreviations)
+ Added additional options for valid methods and headers
+ Moved strings to constants
+ Adjusted multi return functions to single result
+ Resolved logging style warnings
+ Removed vars
+ Fixed variable naming
+ EE added to abbreviations
github-advanced-security[bot]
github-advanced-security bot found potential problems May 26, 2025
View reviewed changes
@robearlam robearlam requested a review from sc-ivanlieckens May 27, 2025 03:27
sc-ivanlieckens
sc-ivanlieckens requested changes May 27, 2025
View reviewed changes
Copy link
Collaborator
@sc-ivanlieckens sc-ivanlieckens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Srry about that one @robearlam. Seeing the new situation I realized it's not optimal for cognitive understanding of the usage as per our teams conversation.

@robearlam robearlam requested a review from sc-ivanlieckens May 27, 2025 07:39
@robearlam robearlam merged commit d4a1d41 into main May 27, 2025
3 checks passed
@robearlam robearlam deleted the feat/17-pages-metadata-editing-mode branch May 27, 2025 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jballe jballe jballe left review comments

@sc-ivanlieckens sc-ivanlieckens sc-ivanlieckens approved these changes

Assignees

@robearlam robearlam

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for XMC Editor Metadata integration
5 participants
@robearlam @jballe @Amir818 @sc-ivanlieckens @IvanLieckens
0