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

Consider stacked h1 pattern for guides #122

Open
willguv opened this issue Aug 2, 2023 · 19 comments
Open

Consider stacked h1 pattern for guides #122

willguv opened this issue Aug 2, 2023 · 19 comments
Assignees

Comments

@willguv
Copy link
Member

willguv commented Aug 2, 2023

The NHS guide page equivalent uses a single h1 for titles: page title above, overview title beneath

https://www.nhs.uk/conditions/acute-pancreatitis/causes/

Image

The content group is enthusiastic about this change

We need to:

  • consider as a default for new installs
  • provide as an option to existing councils (as they may need to make theming changes to properly implement it)
  • discuss whether this should be the default for step by step pages as well (replacing the "part of" panel)

Image

@willguv
Copy link
Member Author

willguv commented Aug 30, 2023

Needs a bit more thought about how to turn on/ off

Challenge is how to roll out and not affect existing sites - not a block, so where does it live? Could be theme option

@ctorgalson
Copy link

ctorgalson commented Sep 7, 2023

@3mkay and I had a think about this, and 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.

In any case, we wanted to get some feedback before creating a PR, but the reasoning goes like this:

  • we currently set the title with $event->setTitle(),

  • setTitle() (like setLede()) will accept a render array,

  • so localgov_guides could provide a hook_theme() implementation and a corresponding page-header.html.twig template,

  • then, setTitle() in the event subscriber could do basically this:

    $event->setTitle([
      '#theme' => 'page_header',
      '#node' => $overview,
    ]);
    

To maintain backwards compatibility, the page-header.html.twig template provided by the module could basically be

{{ node.title }}

...but since we pass in the node, template overrides could use whatever node data (and markup) they need.

The main question I have is what we'd use to handle the differences between Guide Overviews and Guide pages:

  1. logic in the event subscriber, preserving the existing behaviour for Guide Overview pages but permitting customization of Guide Page titles,
  2. template suggestions from the module based on bundle,
  3. just twig logic based on node.bundle,

(1) is probably simplest, (2) is probably most flexible, and (3) is probably most like what's already happening.


Does this sound like a workable solution? I'm not very deep into this module, so I could easily be overlooking something important.

@ctorgalson
Copy link

@willguv Any thoughts on this? We can put together a PR if it seems like a sensible approach to you.

@willguv
Copy link
Member Author

willguv commented Sep 18, 2023

Hi @ctorgalson sorry I hadn't replied to this - thanks for the nudge

Yes please go ahead, very kind of you!

Copying @stephen-cox

@willguv
Copy link
Member Author

willguv commented Oct 10, 2023

Hi @ctorgalson are you at Annertech by any chance? I think Haringey might have asked for this work already

@ctorgalson
Copy link

@willguv I am. Their request was what started us looking at this. I should have a chance to work on it soon.

@willguv
Copy link
Member Author

willguv commented Nov 7, 2023

Hi @ctorgalson hope all's well. Any news on this? Many thanks, Will

@andybroomfield
Copy link
Contributor

See technical discussion and implementation for guides on #133 and possible more generic solution for opt in use on any content type on localgovdrupal/localgov_core#193

@willguv
Copy link
Member Author

willguv commented Nov 8, 2023

We're following NHS' lead by including both the title and lede in the h1

Agreed in the product group, @andybroomfield present

@willguv
Copy link
Member Author

willguv commented Jan 13, 2024

Hi @ctorgalson @markconroy, are you still working on this? Many thanks, Will

@willguv
Copy link
Member Author

willguv commented May 22, 2024

@markconroy would be good to pick this up and finish it if possible

@3mkay
Copy link

3mkay commented May 22, 2024

@willguv my understanding is that there was a change made upstream during our work which means we need to re-work what we’ve delivered on the HLBC site for it to apply.

@willguv
Copy link
Member Author

willguv commented May 22, 2024

Thanks @3mkay - is it a big job?

@andybroomfield
Copy link
Contributor

Instructions in the 2.13 release notes
https://github.com/localgovdrupal/localgov_core/releases/tag/2.13.0

@ctorgalson
Copy link

ctorgalson commented May 22, 2024 via email

@markconroy
Copy link
Member

Here's a PR to add the subtitle variable for localgov_base - localgovdrupal/localgov_base#561

@willguv
Copy link
Member Author

willguv commented Jun 11, 2024

@msayoung I vaguely remember writing this because a council raised accessibility and usability issues with the existing Guides pattern

There was a lot of enthusiasm for the change in the content group

Mark's done so enabling work to make this much easier to implement. Should we go ahead and make this change? If so, should it be a default or an option for councils?

Thanks

@willguv
Copy link
Member Author

willguv commented Sep 25, 2024

@markconroy should we go ahead and implement this change/ give councils an option to use it?

@markconroy
Copy link
Member

Yes, I think it's okay to merge this one. We chatted about it a lot and gone around the implementation a few times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress - Everyone else
Development

No branches or pull requests

5 participants