Make setting the page title format accessible to site administrators

[scleaver]

Currently seeking feedback on my approach to allowing page title format to be moved from the theme layout level to be able to be set by site administrators. This is in response to issue #3400.

Approach

The code is currently not ready for merge but I would like comment on my approach here and see whether people agree. Here it is:

1. I have created a new shape in display management called "PageTitle" which can now be placed between the title tag in a layout like so <title>{{ "PageTitle" | shape_new | shape_stringify }}</title>
2. I have added a new site setting called "PageTitleFormat" and have added a new field to the general site settings under baseurl. The intention here is that the site admin will use liquid to specify the desired final format of the page title. The default format will be {% page_title Site.SiteName, position: "after", separator: " - " %}
3. The PageTitleShape will be responsible for parsing the PageTitleFormat and combining this with the current page segments (this part is not finished yet).

Questions I have

1. Is it possible to not require the shape_stringify so that the use would be {{ "PageTitle" | shape_render }} or is {{ "PageTitle" | shape_new | shape_stringify }} the correct way?
2. How do I parse the {% page_title Site.SiteName, position: "after", separator: " - " %}

Please supply your feedback.

[scleaver]

Ok so I assume I need to use RenderAsync method of the ILiquidTemplateManager - can anyone tell me how in my shape I would go about getting the templatecontext?

[deanmarcussen]

@scleaver Like the idea of a pagetitle shape.
Can I make a suggestion. Could the PageTitle shape be templated rather than provide IHtmlContent directly, and provide the IHtmlContent to the view/alternate and also pass the TitleSegments, so that we could provide alternates and overrides for different areas of the site.
For example I to define a page title format recently in a few different ways to satisfy the seo guy

Agency | The amazing things the agency does (home page)
Agency - Small section | The amazing things agency does (section with not a lot of content items in it)
A title of my blog - Blogs | Agency (section with lots of CI where they wanted the title of the CI first)

If I could have a PageTitle to satisfy most normal requirements then override with Blog-PageTitle it would be great

Re: TemplateContext
I think (maybe) if your shape was described via a IShapeTableProvider - see OC.Contents.Shapes for how a ContentItem is lazily loaded - then in .OnProcessing (or maybe OnDisplaying) you could create a new TemplateContext and pass the current ContentItem into the TemplateContext so you have a TemplateContext to call RenderAsync - but don't know enough about liquid templating to be sure you even need to pass the ContentItem into the TemplateContext in your case. Might be a new TemplateContext() is enough?

[scleaver]

Actually Dean that is great idea!! Often your homepage is a different format to other areas of the site so a template that can vary would be amazing. This might be a bit beyond me though - would it be fairly straight forward to implement?

[deanmarcussen]

Sweet. Yeah they always want something different for the homepage. I reckon if you have a look in OC.Contents.Shapes and do it with an IShapeTableProvider it should be pretty straightforward to do your work in OnProcessing, and describe the alternates in the OnDisplaying . You might have to provide a PageTitle.Summary view that does nothing to stop it getting overwritten by a ContentItem when it's contained in a BagPart and a PageTitle.Detail view that renders for the main CI.

[scleaver]

@deanmarcussen so you would still get the default setting from site settings but have also the ability to override it?

Also I have tried new TemplateContext() but it didn't seem to process the liquid.

Would I still do this in the display management module?

[scleaver]

I don't think I can do this in the display management module because I would have to make it referene the content management modules.

[deanmarcussen]

@scleaver Your original plan was to make default setting available to administrators - so probably a site admin knows how to change settings, but might not know (or is a dev job!) how to build templates and alternates, so I'm thinking with that reasoning probably helpful to have a site setting - IMHO

Think you can use IShapeTableProvider in DisplayManagement - it's described in DisplayManagement.Descriptors so intended to be used from there.

Couldn't say about the new TemplateContext() - it may need a ContentItem set.. something like

            var templateContext = new TemplateContext();
            templateContext.SetValue("ContentItem", part.ContentItem);
            templateContext.MemberAccessStrategy.Register<TitlePartViewModel>();

is enough to get it going in a Driver

Here's a very short version of a shapetableprovider I used in a theme (with Razr), if it helps at all

    public class PricingShapeProvider : IShapeTableProvider
    {
        public void Discover(ShapeTableBuilder builder)
        {
            builder.Describe("PlanTypes");
        }
    }

Rendered with

@await DisplayAsync(await New.PlanTypes(PlanTypes: freePlan, ColumnClasses: "col-12"))

I see that DisplayManagement doesn't actually hold any Razor views, so maybe @sebastienros could chip in with some feedback on best place (and if he thinks this even a good idea to make it templated!)

[sebastienros]

Do you need help with that?

[scleaver]

@sebastienros - I am little stuck on this PR. I will have a look at in the next day and reach out for help.

[scleaver]

Bring up to date with latest dev branch.

[scleaver]

This is needed for #3214

[sebastienros]

I completed this PR with the missing code

[sebastienros]

Can someone check why it doesn't work?

[jtkech]

Just forked the branch, it fails on setup because it resolves the IShapeAttributeProvider and PageTitleShapes inject ISiteService that is not registered on setup.

I will fix it.

[jtkech]

There is still a race condition in LuceneIndexManager while running unit tests, but it is not related to this PR, so i restarted the job on travis

[jtkech]

That's ok now for unit tests.

I didn't keep the trace when it was failing in LuceneIndexManager but, as a reminder, it was failing line 291 when creating a new IndexWriterConfig even if we use here a tenant level lock.

Maybe here we would need a lock on a static object as we already did in another place and where we saw that Lucene using some global dictionary is not thread safe, but after more investigations.

[jtkech]

Can someone check why it doesn't work?

Just to say that it has been fixed