Problem/Motivation

#2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) introduced a site branding block to core. The default label is "Site Branding" but all other block labels (and generally all labels in core) use sentence case instead of title case.

Proposed resolution

Change the label to "Site branding".

Remaining tasks

Review the patch.

User interface changes

The default site branding block label will change.

API changes

None

Data model changes

None

CommentFileSizeAuthor
#8 2566579-branding-block-label-5.patch2.57 KBlongwave
#6 2566579-branding-block-label-5.patch2.57 KBhussainweb
#6 interdiff-2-5.txt441 byteshussainweb
#2 2566579-branding-block-label.patch2.13 KBlongwave

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2566579-branding-block-label.patch2.13 KB
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Needs work

Thanks @longwave, reviewed and I think you missed one, but actually everywhere else this is lower case in comments, so might as well lower case this too:

Visual styles for the Site Branding block in Bartik.

core/themes/bartik/css/components/site-branding.css

star-szr’s picture

star-szr
he/him
Primary language English
commented

Thanks @longwave!

+++ b/core/modules/system/system.install
@@ -1495,7 +1495,7 @@ function system_update_8006() {
-    'settings.label' => 'Site Branding',
+    'settings.label' => 'Site branding',

This needs to happen in a new hook_update_N(), doesn't it?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
StatusFileSize
new 2566579-branding-block-label-5.patch2.57 KB

@joelpittet I wasn't sure of the standard to use in comments but I might as well entirely lowercase it there, fixed in the attached patch.

@Cottser I figured if it was committed soon enough after the parent then the small number of sites that upgraded in the meantime can live with the incorrect label? A new update hook will be more code than the entire rest of this patch.

hussainweb’s picture

hussainweb
Primary language English
commented
StatusFileSize
new interdiff-2-5.txt441 bytes
new 2566579-branding-block-label-5.patch2.57 KB

Addressing #3.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

@hussainweb: I think "site" should be lowercased in the comment, there is no need for it to be capitalised there - already done in #5.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 2566579-branding-block-label-5.patch2.57 KB

Reuploading #5, it was hidden before it could be tested and now I can't unhide it.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

@hussainweb I agree with #7, it's the same as the other instances in comments. Otherwise you need to change the other instances to that as well. Keep lower case. If you disagree feel free to un-RTBC.

RTBC #8

hussainweb’s picture

hussainweb
Primary language English
commented
Issue tags: +Quickfix

Ah, crosspost. I think #7 makes sense. I also think the comment in #5 is justified regarding the hook_update_N. It seems an overkill to write a new update hook for this. So, +1 to RTBC.

star-szr’s picture

star-szr
he/him
Primary language English
commented

Yeah I wasn't sure on the upgrade path policy seems like it's release to release now HEAD to HEAD so we should be good :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

No hook_update_N - an existing sites config is their config - and they can change this label.

Committed 592fe44 and pushed to 8.0.x. Thanks!

  • alexpott committed 592fe44 on 8.0.x 
    Issue #2566579 by longwave, hussainweb: Use sentence case for branding...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.