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

Rendering zones before rendering layout #5436

Merged
merged 9 commits into from
Feb 6, 2020

Conversation

ns8482e
Copy link
Contributor

@ns8482e ns8482e commented Feb 1, 2020

Fixes #3947, #1994, #993

Rendering zones before Layout allow zones to register resources to ResourceManager.

}

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.

// Finally we render the Shape's HTML to the page's output
// Render Shapes in Content
var htmlContent = await DisplayAsync(ThemeLayout.Content);
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 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.


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.

@jtkech
Copy link
Member

jtkech commented Feb 2, 2020

@ns8482e so the suggestion would be

    public override async Task ExecuteAsync()
    {
        // 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);

            if (ThemeLayout is ZoneHolding zones)
            {
                foreach (var zone in zones.Properties.ToArray())
                {
                    if (zone.Value is Shape shape)
                    {
                        // Pre-render each layout zone
                        var htmlZone = await DisplayAsync(shape);
                        zones.Properties[zone.Key] = htmlZone;
                    }
                }
            }

            // Finally we render the Layout Shape's HTML to the page's output
            Write(await DisplayAsync(ThemeLayout));
        }
        else
        {
            Write(body);
        }
    }

Then in RazorPage.cs.RenderSectionAsync() in place of checking required && zone != null && zone.Items.Count == 0 e.g check required && zone is Shape && zone.Items.Count == 0.

Then as said, in both solutions maybe one drawback if in the layout template someone want to deal with the items of a given zone.

@ns8482e
Copy link
Contributor Author

ns8482e commented Feb 2, 2020

Replaced Zone shapes with IHtmlContent as suggested above.

Copy link
Member

@jtkech jtkech left a comment

Choose a reason for hiding this comment

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

Please remove the remaining unused usings

At the end there are 3 things i'm used to do

  • Right click on the usings to organize them and remove the unused ones

  • Ctrl K+D for formatting

  • Add a new line at the end of the file

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.

Rendering stylesheets with style tag helper from widgets
4 participants