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

Add stacked header subtitle as new page header variable #193

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

andybroomfield
Copy link
Contributor

@andybroomfield andybroomfield commented Nov 2, 2023

Fix #192

This allows page subscribers to set a subtitle which will be stacked inside the h1.

Ref #192

This allows page subscribers to set a subtitle which will be stacked
inside the h1.
@ctorgalson
Copy link
Contributor

Coming over from the PR 133 on localgov_guides.

I like this (though I would quibble pointlessly about that subtitle markup 😀). I have two additional suggestions:

  1. add another variable, node, to the module's hook_theme implementation for this block, then populate it with the node object. This will make it simple, in themes, to override the rendering of the block according to content type (and according to the node content itself),
  2. add template suggestions for the content type to the block template (maybe using EntityTypeBundleInfo() in hook_theme_suggestions_HOOK()...not sure if we would want to limit this to LGD content types, or just catch them all),

The fact that what might be relatively simple things to handle in themes spawn issues like consider stacked h1 pattern for guides, and that (as you pointed out) localgov_publications uses a different block suggest to me that the core block could be more helpful specifically w/r/t the current content type.

@andybroomfield
Copy link
Contributor Author

Thanks for the suggestions @ctorgalson. I'll look into adding the entity, thats actully one thing to check over as it should be the case the page title block can be used with any entitiy and not just node, and in fact even stand alone controllers (we use it at BHCC to modify search to include the search terms).
However getting the current node shouldn't require getting the route path, so if its avaliiable we should provide it, and perhaps provide a way for other modules to set which entitiy is avalible.

- Add subtitle test condition and amendment to test page header
- Use xpath to get the header, as sometimes page title is found on page as it is the title tag.
  also, the adjustment of the template means the exact markup for the h1 breaks with newlines.
@andybroomfield andybroomfield marked this pull request as ready for review November 18, 2023 17:10
@andybroomfield
Copy link
Contributor Author

I'm marking this as ready for review, although this is just the adding of the subtitle.
@ctorgalson I think we should add the entity to the template and template suggestions as a new PR as that is adding some new functionality and needs some thought on implementation and tests. This should be sufficient to then cover most use cases for the page header (title with lede, title with subtitle inside h1).

@markconroy
Copy link
Member

This looks good to me.

@ctorgalson
Copy link
Contributor

It looks good to me too 👍

@andybroomfield andybroomfield merged commit fde05ad into 2.x Nov 20, 2023
8 checks passed
@andybroomfield andybroomfield mentioned this pull request Nov 20, 2023
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.

Support adding a subtitle inside the h1 in page header block
3 participants