Skip to content
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

Make it possible to add Razor HTML content to the shape as a property #14867

Merged
merged 47 commits into from
Jul 24, 2024

Conversation

sarahelsaig
Copy link
Contributor

@sarahelsaig sarahelsaig commented Dec 8, 2023

With this change, you can pass HTML content as shape properties using the <add-property name="propertyName"> child tag helper. The rendered inner HTML will be assigned to the given propertyName. This can be used to easily include complex HTML content in a reusable shape.

To test, enable the Orchard Demo module and go to ~/OrchardCore.Demo/Home/AddProperty. It will look like this:
image

which corresponds to this code:

<shape type="EmbedContentInShape">
    <add-property name="HtmlContent">
        The inner HTML of the <code>&lt;add-property name="propertyName"&gt;</code> tag helper (which is a direct child
        of the <code>&lt;shape&gt;</code> tag helper) is converted into HTML and then passed to the shape as a model
        property. The property's name is the string passed into the <code>name</code> attribute.
        <shape type="TestContentPartA" Line="Sample Data" Creating="Now" Processing="Later"/>
        Even other shapes can be included!
    </add-property>
    <add-property name="OtherContent">
        You can have multiple, they are just <code>@nameof(IHtmlContent)</code> type shape properties.
    </add-property>
    <add-property name="SomeProperty" value="Some value passed to the attribute." />
</shape>

<shape type="EmbedContentInShape" HtmlContent="This property is ignored.">
    <add-property name="HtmlContent">
        If a <code>&lt;add-property&gt;</code> tag helper exists, it takes precedence over any matching attribute of the
        <code>&lt;shape&gt;</code> tag helper. This is because child tag helpers are evaluated after the shape is
        created, right before it's displayed.
    </add-property>
</shape>

(EDIT: updated description to represent the current state of the PR. That is using a child tag helper instead of directly using the inner HTML, so doesn't conflict with the existing <metadata> tag helper and multiple can be used.)

For some reason the original code kept crashing, saying something along the lines of IShape doesn't have "Line" method.
@Piedone
Copy link
Member

Piedone commented Jan 3, 2024

Could you please bring this up on the Thursday meeting?

@ns8482e
Copy link
Contributor

ns8482e commented Jan 3, 2024

Not sure what value it would add to have shape tag with content? why not just use the content in div?

It's defeat the purpose of ShapeTagHelper. ShapeTagHelper helps to render a given shape harvested from any module or app- but itself is not a Shape.

@sarahelsaig
Copy link
Contributor Author

Could you elaborate on what you mean by "just use the content in div"? Is there already an intuitive way to pass Razor HTML content into a shape? If so, I couldn't find it so there may be a discoverability problem.

What prompted this contribution: I wanted to make a reusable carousel shape, where the shape contains the frame and the script, but the content is customized by the invoking code. This would save a ton of boilerplate for this use-case. Something like this:

Carousel.cshtml:

<style asp-name="@ResourceNames.SlickTheme"></style>
<script asp-name="@ResourceNames.Slick" at="Foot"></script>

<div id="@Model.CarouselId" class="carousel__wrapper">
    <div class="carousel">
        @Model.Content
    </div>
    <div class="carousel__arrows"></div>
</div>

<script at="Foot" depends-on="@ResourceNames.Slick">
    jQuery(function ($) {
        $("#@Model.CarouselId").slick({
            // parameters to init Slick
        });
    });
</slick>

Some page that wants to display carousels:

<h2>@T["Product Highlights"]</h2>
<shape type="Carousel" prop-CarouselId="productCarousel">
    @foreach (var shape in productShapes)
    {
        <div class="carousel__item productCarousel__item">
            @await DisplayAsync(shape)
        </div>
    }
</shape>

<h2>@T["Gallery"]</h2>
<shape type="Carousel" prop-CarouselId="galeryCarousel">
    @foreach (var (url, altText) in images)
    {
        <div class="carousel__item galeryCarousel__item">
            <img src="@url" alt="@altText" />
        </div>
    }
</shape>

As you can see, passing the content into the shape is very versatile. It would be much more complicated to achieve this if I had to manually construct the IHtmlContent in C# code or if I had to make separate shapes for different types of carousel items. The same logic would apply for all container style shapes.

@ns8482e
Copy link
Contributor

ns8482e commented Jan 3, 2024

In shape @Model.Content is not just a property, it's really a Location that you can define in display drivers. Looks like you are not using any display driver that's why you are seeing Content as a property. Use driver instead

@sarahelsaig
Copy link
Contributor Author

sarahelsaig commented Jan 3, 2024

That's not related to what I'm trying to do. I just want to pass HTML content to the shape.
Maybe calling the property Content is confusing? I've renamed it to HtmlContent. Is this better?

@ns8482e
Copy link
Contributor

ns8482e commented Jan 3, 2024

You can define any property to your shape as per your need, but should not change the purpose of the ShapeTagHelper.

ShapeTagHelper is not shape or content itself it helps render the shape define by the type

Define your content in Carousel.cshtml or in another shape that you can call from Carousel.cshtml

@sarahelsaig
Copy link
Contributor Author

ShapeTagHelper is not shape

You have already said this and I don't understand why, or why you think anything in this PR changes the purpose of the ShapeTagHelper. Can you please elaborate on this complaint?

It just adds another way to pass in an IHtmlContent type shape property from the tag content. This is functionally no different to using tag attributes. Except of course the shape property has to have a fixed name due to HTML limitations.

Define your content in Carousel.cshtml or in another shape that you can call from Carousel.cshtml

I don't have to do that. I can replicate my above sample using existing features already, it's just extremely clunky:

@{
    var productCarouselInner = new HtmlContentBuilder();
    foreach (var shape in productShapes)
    {
        productCarouselInner
            .AppendHtml("<div class=\"carousel__item productCarousel__item\">")
            .AppendHtml(await DisplayAsync(shape))
            .AppendHtml("</div>");
    }

    var galleryCarouselInner = new HtmlContentBuilder();
    foreach (var (url, altText) in images)
    {
        galleryCarouselInner
            .AppendHtml("<div class=\"carousel__item galleryCarousel__item\">")
            .AppendHtml($"<img src=\"{ url }\" alt=\"").Append(altText).AppendHtml("\" />")
            .AppendHtml("</div>");
    }
}

<h2>@T["Product Highlights"]</h2>
<shape type="Carousel" prop-CarouselId="productCarousel" prop-HtmlContent="@productCarouselInner"></shape>

<h2>@T["Gallery"]</h2>
<shape type="Carousel" prop-CarouselId="galeryCarousel" prop-HtmlContent="@galleryCarouselInner"></shape>

Of course this way I can't use tag helpers, have to be careful about sanitizing HTML, and the IDE doesn't provide nice Razor syntax highlighting. So in a sense my PR is primarily a usability and tooling improvement. Again, it does not change what the shape tag helper is, it only adds an additional, backwards compatible, non-intrusive way to define a specific property.

@ns8482e
Copy link
Contributor

ns8482e commented Jan 4, 2024

I understand your need, but you are using ShapeTagHelper as Slots. Shapes are not same as slots.

Help me understand What happens to the slots content when alternates/wrappers are added to the Shape?

In your code example, the slot content you are adding, to me it's just a wrappers of your product/gallery shape. You can add wrapper to your shape to achieve same without modifying ShapeTagHelper.

Btw, You can achieve same by using display driver. Which is more elegant and extensible- support wrappers and alternates.

Following some pseudo logic of driver that you could use.

public override IDisplayResult Display(CarouselModel model)
        {
var listShapes  = new List<IDisplayResult>();

foreach (var item in model.Products)
    {

var ps = Dynamic("CarouselProduct", shape =>
            {
                shape.Product = item;
            })
            .Location("Detail", "Content");

listShapes.Add(ps):

        }

foreach (var galleryItem in model.Images)
    {
       var gs = Dynamic("CarouselGallery", shape =>
            {
                shape.GalleryItem = galleryItem;
            })
            .Location("Detail", "Gallery");

listShapes.Add(gs):
    }

return Combine(listShapes);
}

@sarahelsaig
Copy link
Contributor Author

This isn't real slots, it just uses the same syntax because it's intuitive. Again, it just passes compiled HTML as a shape parameter, because there is currently no way to pass in HTML Razor into the shape. By the time the shape receives this content it's already built according to the current context and "frozen" so it's just another parameter.

Also it's not trying to replace drivers. I agree that if I wanted to make a generic carousel feature I would write a driver and model. But the carousel was just the latest example where this would've came in handy. The point is to use this if you want to create some project-specific reusable elements with an HTML parameter that won't have to be fleshed out for every possible scenario. In other words, it's for the same situations where you use ad-hoc shapes anyway.

Can we take a step back? I don't understand what's the harm you see in this PR. Why are you so opposed to modifying ShapeTagHelper? It's backwards-compatible and non-breaking.

@ns8482e
Copy link
Contributor

ns8482e commented Jan 5, 2024

Separation of concern.

ShapeTagHelper helps to render a given shape, it is not a shape itself and it doesn't own a shape.

Hence ShapeTagHelper should not add/update arbitrary property to a shape type being rendered.

Using ShapeTagHelper You can pass properties values to shape using prop-*

@sarahelsaig
Copy link
Contributor Author

Wait, so your problem is only that the property name is fixed? Because otherwise there is no difference between this and prop-*, it's only syntax.

@ns8482e
Copy link
Contributor

ns8482e commented Jan 6, 2024

I don't have any issue with the feature or need, the only concern is the current proposed solution is not good way to solve the problem

@ns8482e
Copy link
Contributor

ns8482e commented Jan 6, 2024

Because otherwise there is no difference between this and prop-*, it's only syntax.

There is. - in prop-* ShapeTagHelper is not owning the properties being added. It's just adding what's provided as input, on behalf of the caller.

@sarahelsaig
Copy link
Contributor Author

What do you mean by "owning"? If you look at the code here, you can see that it literally just adds an extra property. In every sense except syntax this is the same as a prop-* attribute

@ns8482e
Copy link
Contributor

ns8482e commented Jan 6, 2024

I think better solution is to add a new child TagHelper of ShapeTagHelper similar to how AddAlternateTagHelper is.

Let's say call it SlotContentTagHelper having input prop-name and that would add content of TagHelper as property of the shape.

So proposed Template would look like below.

<shape type="Carousel" prop-CarouselId="productCarousel">
<slot-content prop-name="MyProp">
    @foreach (var shape in productShapes)
    {
        <div class="carousel__item productCarousel__item">
            @await DisplayAsync(shape)
        </div>
    }

</slot-content>

</shape>

@sarahelsaig
Copy link
Contributor Author

sarahelsaig commented Jan 6, 2024

I think better solution is to add a new child TagHelper of ShapeTagHelper similar to how AddAlternateTagHelper is.

We could add a virtual method like protected virtual Task OnPropertiesAddedAsync(IDictionary<string, object> properties, TagHelperContext tagHelperContext, TagHelperOutput output) that would be invoked instead of this code and then it can be overwritten in an inherited class. It would also improve the extensibility of the base class. Is this suitable?

Let's say call it SlotContentTagHelper having input prop-name and that would add content of TagHelper as property of the shape.

Ok I will look into it...

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

Thank you.

I'm asking this the third time, so please reply: Did you bring it up during the Thursday meeting? Or even the Tuesday one would be good.

@sarahelsaig
Copy link
Contributor Author

Did you bring it up during the Thursday meeting? Or even the Tuesday one would be good.

afair I did not bring this up in a meeting.

@hishamco
Copy link
Member

Did you bring it up during the Thursday meeting? Or even the Tuesday one would be good.

I will try to bring this tomorrow if I attend in case Sara can't, also you can contact Gabor coz he is almost the time there

@sebastienros
Copy link
Member

We had something called "capture" in O1 for this I believe, and there is a liquid tag called capture too which is documented. I am pretty sure we also have it for Razor but can't find an example right now.

@sarahelsaig
Copy link
Contributor Author

Is this an example of the capture you mentioned?

@using (Capture(Layout.Messages)) {
    <div id="save-message" class="message message-Warning" style="display:none">@T("You need to hit \"Save\" in order to save your changes.")</div>
    <div id="start-message" class="message message-Warning" style="display:none">@T("The workflow needs at least one activity to be set as a starting state.")</div>
}

This feels more similar to the <zone> tag helper. At least the above example would be ported to OC like this:

<zone name="Messages">
    <div id="save-message" class="message message-Warning" style="display:none">@T["You need to hit \"Save\" in order to save your changes."]</div>
    <div id="start-message" class="message message-Warning" style="display:none">@T["The workflow needs at least one activity to be set as a starting state."]</div>
</zone>

@sebastienros
Copy link
Member

@sarahelsaig Correct. Different than your suggestion?

@sarahelsaig
Copy link
Contributor Author

sarahelsaig commented Jul 1, 2024

Yes, <zone> is for appending HTML to a global zone of the layout.
But <add-property> if for adding HTML to a shape as a property, so arbitrary content can be displayed inside the shape template.

@Piedone
Copy link
Member

Piedone commented Jul 14, 2024

@sebastienros this is waiting for your reply.

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@Piedone
Copy link
Member

Piedone commented Jul 22, 2024

@sebastienros please reply here. I'll merge it on Wednesday if there's no further feedback.

Also, @sarahelsaig, please fix the merge conflict. I know, it feels pointless when the PR is just sitting there, but now it won't :).

@sarahelsaig
Copy link
Contributor Author

I'll merge it on Wednesday if there's no further feedback.

ahhhh-its-wednesday-my-dudes

@hishamco
Copy link
Member

I will update the branch and then merge, no worries :)

@hishamco hishamco merged commit 79a0853 into OrchardCMS:main Jul 24, 2024
6 checks passed
@hishamco
Copy link
Member

ShipIt

@sarahelsaig

@Piedone
Copy link
Member

Piedone commented Jul 24, 2024

I had a timer saved for this that went off half an hour ago, so would've merged now, but OK then :D.

@hishamco
Copy link
Member

I'm trying to avoid making PRs take so long, that's why I need to revise my PRs one more time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants