122 consider stacked h1 pattern for guides

[ctorgalson]

This change adds a new optional way to render the title info of Guide Pages (leaving Guide Overviews untouched). The new method renders something like this:

<h1>Guide page title</h1>
<p>Guide overview title</p>

Code-wise, what we're doing is this:

• adding a new settings form (along with the usual route & menu item) with one field that toggles rendering between the current method (the default), and the new (Twig-based) method,
• adding two templates to templates/ and to hook_theme(),
• modifying the event subscriber to render the block title and lede as described above only when the new field is not set.

Note that localgov_base doesn't properly render the 'new' version because it directly accesses lede['#value'] instead of just printing lede. This means that, for the moment, this change requires a project using it to override localgov_base/templates/block/localgov-page-header-block.html.twig.

If localgov_base printed the variable like this instead, it'd solve the issue--I'll propose it on the relevant repo:

{% if lede %}
  {% if lede['#value'] %}
    <p class="lgd-page-title-block__subheader">{{ lede['#value'] }}</p>
  {% else %}
    {{ lede }}
  {% endif %}
{% endif %}

[andybroomfield]

If a site hasn't explicity set to use the legacy header, this will automatically opt them into the new style until they save the config form. Maybe consider reversing the check to see if they want new style headers? Or make sure to include an update hook for existing sites to set this as legacy header. Note the settings form below does assume legacy header as default, but it won't apply until its saved.

[ctorgalson]

Thanks, I missed that (saved the form before ever testing). I've taken your first suggestion to require the checkbox to be set to use the 'modern' rendering. That means the default, unsaved state uses the existing rendering (since it defaults to FALSE).

[andybroomfield]

Getting the settings from the config factory in the constructor is considered an anit-pattern. Instead get the config factory in the constructor and then get the settings when needed. We ran into an interesting issue with this at BHCC with a contrib module causing some strange side effects.

[ctorgalson]

Good to know. Fixed.

[andybroomfield]

I think Content authoring would be a better parent? Would be interested in others opinions.

[ctorgalson]

Me too. I couldn't figure if there was a standard localgovdrupal approach for these sorts of settings.

[andybroomfield]

Just checking something, appreciate this is still being worked on.
Testing with both this PR and localgovdrupal/localgov_base#492

I notice that it's not wrapping everything in the h1, insead setting the title of the guide overview as a lede.
I'm trying to work out why there is a need to change the rendering of the page heaer block, though I do think it could do with being made more generic.

Looking at the h1 pattern on the NHS site example, it the Guide page title and Guide overview are wrapped in a single h1. It looks like we should be able to achive the same effect by setting this to the setTitle (and passing a render array for the subtitle, or adding subtitle support to the core page header block) and either setLede to blank or allow an option to allow the page summary to be displayed as normal.

[andybroomfield]

So I was doing a quick expriment and this works (though would need theming and there are prob better ways of doing this).

      $event->setTitle([
        'title' =>[
          '#type' => 'markup',
          '#markup' => $node->getTitle(),
        ],
        'subtitle' => [
          '#type' => 'markup',
          '#markup' => '<span class="subtitle"> ' . $overview->getTitle() . '</span>',
        ],
      ]);
      $event->setLede(NULL);

[ctorgalson]

Just checking something, appreciate this is still being worked on. Testing with both this PR and localgovdrupal/localgov_base#492

I notice that it's not wrapping everything in the h1, insead setting the title of the guide overview as a lede. I'm trying to work out why there is a need to change the rendering of the page heaer block, though I do think it could do with being made more generic.

We can't influence that from here. localgov_base currently renders this as

<div class="lgd-page-title-block">
  <h1 class="lgd-page-title-block__title">Title content</h1>
  <p class="lgd-page-title-block__subheader">Lede content.</p>
</div>

...while localgov_core renders it as

<div class="clear-both headers key">
  <h1 class="header">Title content</h1>
  <div class="subheader"><p>Lede content</p></div>
</div>

These are close to the whatwg recommendation for heading/subheading markup (though that surrounding <div> should probably be an <hgroup> (since it's apparently been brought back from deprecation).

Looking at the h1 pattern on the NHS site example, it the Guide page title and Guide overview are wrapped in a single h1. It looks like we should be able to achive the same effect by setting this to the setTitle (and passing a render array for the subtitle, or adding subtitle support to the core page header block) and either setLede to blank or allow an option to allow the page summary to be displayed as normal.

This is what I was getting at in #122 when I said "...I think we have a workable solution, assuming that the place to solve the problem is in the individual modules that consume the page header block, and not at the module that provides it."

Given the EventSubscriber is using localgov_core's API (->setTitle() and ->setLede()), I wasn't able to think of a better solution than using Twig templates that can be overridden to provide the content variables for the block.

---

Added: sorry, I didn't answer this question:

I'm trying to work out why there is a need to change the rendering of the page heaer block

Strictly speaking, there isn't. But #122, and the fact that localgov_base implements its own anti-pattern just to add a class to the subheading suggest a need for more flexibility. We can provide that via Twig templates as in this PR, but we can't do it without requiring modification to localgov_base's approach, either in the theme itself, or in templates that override it (this is only true of localgov_base -- localgov_core isn't affected).

This change doesn't require localgov_base to accept my PR over there, but it would make the theming smoother if they did.

[andybroomfield]

Thanks for the explination @ctorgalson.
I think first we need to clarify (as LGD) if we want to have a consistent approach with the other modules (see @finnlewis comment on localgovdrupal/localgov_base#492 (comment)).
What I meant above, not sure if you saw mhy comment with the sample code, that it is possible to have the title and sub title within the h1 by setting the render array on the title only, and not setting the lede at all.

@finnlewis I notice that localgov_publications uses a completly sepearte block and template, not the localgov_core page header block.

[finnlewis]

Thanks for all the thought on this.

Discussing in Merge Monday meeting, I think I'd like to have a think about how we might standardise this across all relevant content types.

@markconroy have you had a chance to think about this?

We also need to fix the tests.

[markconroy]

I think this approach looks really good (along with the corresponding PR in localgov_base. In terms of standardising across other content types, this looks like a sensible standard for us to follow.

I'm not overly convinced about having a checkbox for "use legacy header" vs "use modern header". That looks like we might in future be adding even more checkboxes for things like this such as "use even more modern header", but am not sure how we get around this issue either without breaking changes. Perhaps we can have this change only for current sites and hide that checkbox on new installs? But for now, I'm happy with this.

[ctorgalson]

I think this approach looks really good (along with the corresponding PR in localgov_base. In terms of standardising across other content types, this looks like a sensible standard for us to follow.

@markconroy Arguing against myself, the more standardis-y approach might be to do this (the Twig template) in the original block in localgov_core, but that would mean all modules that alter the block output would need to change to adapt to that change. If localgov_guides is the only one, that would be ok, but if lots of modules work on the block it could be a bunch of works.

I'm not overly convinced about having a checkbox for "use legacy header" vs "use modern header". That looks like we might in future be adding even more checkboxes for things like this such as "use even more modern header", but am not sure how we get around this issue either without breaking changes. Perhaps we can have this change only for current sites and hide that checkbox on new installs? But for now, I'm happy with this.

It's only a way to expose a boolean config entry. For now, it maintains the status quo, but in a major version release, an update hook could reverse the default setting so that new installs would get the 'modern' rendering.

[ctorgalson]

@finnlewis @andybroomfield Any further thoughts about this approach?

[andybroomfield]

I've put my proposal as a draft PR localgovdrupal/localgov_core#193.
This allows modules to optionally set a subtitle element, though any theme with its custom implementation of the block template will need to account for it.

[ctorgalson]

@andybroomfield

I've put my proposal as a draft PR localgovdrupal/localgov_core#193. This allows modules to optionally set a subtitle element, though any theme with its custom implementation of the block template will need to account for it.

I'm happy to see that (IMO the best place to address this is in the block-provider localgov_core). But it doesn't directly address this issue. Should we put this one to the side until the localgov_core changes are stable?

[andybroomfield]

If there is a need to get this in soon for guides we could go with the solution here and then develop the common solution in Localgov_core. Since to use the solution in core this would need a new release anyway which could then require a minimum version of localgov_core to work with once its done. either way localgov_base needs it's fix applied.

[finnlewis]

Sorry @ctorgalson - this seems to have lost momentum and we have some conflicts.

Are we still looking to fix this and get this merged?