-
Notifications
You must be signed in to change notification settings - Fork 8
Changed text based TagHelpers to rendering Chromes with internal span not div #40
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
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 fixes a bug where text-based TagHelpers incorrectly rendered chrome wrappers using div tags instead of span tags, which caused rendering issues when placed inside a p tag. Key changes include:
- Replacing with for opening and closing chrome wrappers in both TextFieldTagHelper and RichTextTagHelper.
- Updating corresponding unit tests to expect tags.
- Reordering using directives in RichTextTagHelperFixture for consistency.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
tests/Sitecore.AspNetCore.SDK.RenderingEngine.Tests/TagHelpers/Fields/TextFieldTagHelperFixture.cs | Updated assertion to expect wrappers in rendered output |
tests/Sitecore.AspNetCore.SDK.RenderingEngine.Tests/TagHelpers/Fields/RichTextTagHelperFixture.cs | Updated assertion to expect wrappers and reordered using directives |
src/Sitecore.AspNetCore.SDK.RenderingEngine/TagHelpers/Fields/TextFieldTagHelper.cs | Changed chrome wrapper tag from div to span |
src/Sitecore.AspNetCore.SDK.RenderingEngine/TagHelpers/Fields/RichTextTagHelper.cs | Changed chrome wrapper tag from div to span |
Comments suppressed due to low confidence (1)
tests/Sitecore.AspNetCore.SDK.RenderingEngine.Tests/TagHelpers/Fields/RichTextTagHelperFixture.cs:1
- [nitpick] The reordering of using directives in this test file differs from other files; please confirm that this rearrangement aligns with the project's coding style guidelines.
using AutoFixture;
src/Sitecore.AspNetCore.SDK.RenderingEngine/TagHelpers/Fields/TextFieldTagHelper.cs
Show resolved
Hide resolved
src/Sitecore.AspNetCore.SDK.RenderingEngine/TagHelpers/Fields/RichTextTagHelper.cs
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.
So we're using a text level semantic element to group content? A rich text element can contain a lot of various elements, lots of them being invalid inside of a span afaik? It also changes the default style from block to inline... This feels wrong?
.../Sitecore.AspNetCore.SDK.RenderingEngine.Tests/TagHelpers/Fields/RichTextTagHelperFixture.cs
Outdated
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.
Approving as temp solution to unblock. JSS and Pages Team contacted for proper resolution.
This fixes the bug where Text based TagHelpers can't render inside of a
p
tag, causing all of MetaData editing to break.This aligns with the JSS approach of using a
span
tag instead.I also introduced a new
Assert
to the appropriate UnitTests as that tag insertion wasn't being tested currently.Testing
Terms
Closes #39