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.

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

Rendering zones before rendering layout #5436

Merged
merged 9 commits into from
Feb 6, 2020
Merged

Rendering zones before rendering layout #5436

Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b2c1327
Fixes #3947, #1994, #993
ns8482e Feb 1, 2020
af182d4
review commetns
ns8482e Feb 2, 2020
d375fe2
fix compile error
ns8482e Feb 2, 2020
6823c98
fix spacing added await
ns8482e Feb 2, 2020
d7d2519
review feedback
ns8482e Feb 2, 2020
5ca519c
fix RenderSectionTag for liquid
ns8482e Feb 2, 2020
27c145c
changed to is IShape
ns8482e Feb 3, 2020
5f3474a
formatting / remov unused using
ns8482e Feb 4, 2020
82fd83d
removed unused using
ns8482e Feb 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 36 additions & 4 deletions src/OrchardCore/OrchardCore.DisplayManagement/Theming/ThemeLayout.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
using System;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Html;
ns8482e marked this conversation as resolved.
Show resolved Hide resolved
using OrchardCore.DisplayManagement.Shapes;

namespace OrchardCore.DisplayManagement.Theming
{
/// <summary>
/// This class represents a precompiled _Layout.cshtml view that renders a
/// This class represents a precompiled _Layout.cshtml view that renders a
/// Layout shape and the View's body in its Content zone.
///
///
/// 1- Views look for any _ViewStart.cshtml
/// 2- <see cref="ThemingViewsFeatureProvider"/> has registered <see cref="ThemeViewStart"/> as the top one
/// 3- <see cref="ThemeViewStart"/> then set a special Layout filename as the default View's Layout.
Expand All @@ -16,15 +20,43 @@ public class ThemeLayout : Razor.RazorPage<dynamic>
{
public override async Task ExecuteAsync()
{
// The View's body is rendered
// The View's body is rendered
var body = RenderLayoutBody();

if (ThemeLayout != null)
{
// Then is added to the Content zone of the Layout shape
ThemeLayout.Content.Add(body);

// Finally we render the Shape's HTML to the page's output
// Render Shapes in Content
var htmlContent = await DisplayAsync(ThemeLayout.Content);
ns8482e marked this conversation as resolved.
Show resolved Hide resolved
var content = ((Shape)ThemeLayout.Content);
Copy link
Member

Choose a reason for hiding this comment

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

Remove useless opening / closing parenthesis => (Shape)ThemeLayout.Content


foreach (var item in content.Items.ToArray())
{
content.Remove(((IShape)item).Metadata.Name);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, i don't think you need to remove the item here because below you will pre-render all zones (the content zone included) and then remove their items (also those of the content zone).

Notice that here the previously added body item is a PositionWrapper whose metadata name and type are null, so here we remove it but only by "matching" a null name.

}

content.Add(htmlContent);
var zoneKeys = ((Zones.ZoneHolding)ThemeLayout).Properties.Keys.ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

Add using OrchardCore.DisplayManagement.Zones and then here just use ZoneHolding

Are we sure that it is always a ZoneHolding, here i think so, but more globally when it is useful maybe better to use if (ThemeLayout.Content is Shape content), if (ThemeLayout is ZoneHolding zones) and so on, so that you check the type and then you can use the new typed variable afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, This will always be zone holding as ThemeLayout class is for pre-compiled Layout. However you suggestion makes more sense.


if(zoneKeys != null && zoneKeys.Count() > 0)
{
foreach (var key in zoneKeys)
{
// Render each layout zone
var zone = ThemeLayout[key];
var htmlZone = await DisplayAsync(zone);
ns8482e marked this conversation as resolved.
Show resolved Hide resolved

foreach (var item in zone.Items.ToArray())
{
zone.Remove(((IShape)item).Metadata.Name);
Copy link
Member

Choose a reason for hiding this comment

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

So, as said above, the problem here is that an item may not have a name as the PositionWrapper, or e.g in the blog theme in the footer zone there is also a WidgetWrapper whose type is Widget_Wrapper but that has a null name. But as said above it may work but only by "matching" null names.

Just tried it works even we don't remove any item and moreover without doing below zone.Add(htmlZone). I think because in DefaultHtmlDisplay there is this check

            if (shape.Metadata.ChildContent == null)

And when we pre-render a zone the ChildContent is already populated and then it is not processed again, but the other handlers are executed again, even if we still remove the items that anyway will not be rendered.

Hmm, the solution would be to replace the zone completely with the related pre-rendered IHtmlContent this because when there is no related shape we do CoerceHtmlString() that just return the object as is if it is an IHtmlContent, so the handlers would not be executed again.

Hmm, but when rendering a section with required: true we check that if the zone is not null we assume that it is a shape and then we check that it has at least one item, so it would fail here because it would not be a shape. Hmm, maybe also check that it is an IHtmlContent or just check that zone is not null.

One drawback i see with both solution if if someone would want in the layout template to deal with the items of a given zone based on some logic, but i think it would be a rare use case.

I will check the code and maybe suggest something.

}

zone.Add(htmlZone);
}
}
// Finally we render the Layout Shape's HTML to the page's output
ns8482e marked this conversation as resolved.
Show resolved Hide resolved
Write(await DisplayAsync(ThemeLayout));
}
else
Expand Down