Skip to content

Conversation

@markconroy
Copy link
Member

Closes #869

What does this change?

places site-slogan inside .branding class to it properly follows our BEM naming conventions.


Thanks to Big Blue Door for sponsoring my time to work on this.

@simonmwhite
Copy link

Hi Mark.

I have reviewed the MR and it looks fine to me apart from the Twig template is using tabs instead of spaces which does not follow Drupal Coding Standards.

Here's what I did to test and what I found. I can see that the logo and slogan are now within .branding class and there is an update to the conditional logic.

I have:

  • reviewed the MR
  • tested toggling the slogan and logo

Recommendation:

  • fix the DCS issue

Thank you.

@markconroy
Copy link
Member Author

Thanks @simonmwhite

Can you mark this as approved if you're okay with it?

I'm not sure why GitHub is reporting tabs instead of spaces, I have my editor set to spaces, and the file is reporting spaces locally.

If it's an issue after we merge it, we can create a follow-up to fix that then.

Screenshot 2025-10-25 at 11 53 46

@simonmwhite
Copy link

Hi @markconroy.

Thanks for sharing the screenshot, I can see you're IDE setup as I'd expect so no worries there. I am going to approve the PR.

@tonypaulbarker and @FinnERLewis one thing that came to mind is that that we do not know which councils have already filled out the slogan and have set this to not show, I'd expect the conditional logic around branding__item--slogan to protect it from showing, that and the config setting. I've not tested this scenario so we could potentially see it appear.

Thank you.

Copy link

@simonmwhite simonmwhite left a comment

Choose a reason for hiding this comment

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

I could see from @markconroy screenshot two spaces are set in his IDE. I am approving this PR.

@finnlewis finnlewis merged commit cf2a57f into 2.x Oct 28, 2025
27 of 29 checks passed
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.

site slogan is outside .branding div

5 participants