Problem/Motivation

In the Seven theme the region that contains the title is called the 'branding' region, I think this is a hangover from days when you could insert an logo in there and have it represent the branding of the organisation. This doesn't make sense from a 'design component' point of view, as we shouldn't be naming components based on their (possible) content. I've also never seen anyone do this in.

Proposed resolution

Rename the CSS file and the CSS classes from 'branding' to 'content-header'

Screenshots on comment: https://www.drupal.org/node/2337379#comment-9208119

Remaining tasks

None

User interface changes

None

API changes

Minor. Changed a class name in seven theme.

Comments

derjochenmeyer’s picture

Status: Active » Needs review
StatusFileSize
new2.01 KB

Here's a patch.

lewisnyman’s picture

Status: Needs review » Needs work

Great thanks! As we're changing everything, do you think we could switch from an id to a class? Kill all the IDs!

derjochenmeyer’s picture

Status: Needs work » Needs review
StatusFileSize
new2.02 KB

Sure, if its just killing that single ID :) Here's an updated patch.

lewisnyman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +dreammarkup
StatusFileSize
new256.73 KB

Thanks! Here's a screenshot of the content header region looking the same :-)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

So then branding.css needs to be renamed or merged to another css file

lewisnyman’s picture

Doh of course, we need to rename it content-header.css

Palashvijay4O’s picture

Submitting a patch .

Palashvijay4O’s picture

Status: Needs work » Needs review
derjochenmeyer’s picture

Status: Needs review » Needs work

How do we rename files using a patch in Drupal? Delete old + create new? Or anything like "git mv"?

It seems patch #7 just deletes branding.css without merging the content or creating a new file called content-header.css

chandeepkhosa’s picture

Assigned: Unassigned » chandeepkhosa

..

chandeepkhosa’s picture

Assigned: chandeepkhosa » Unassigned
Status: Needs work » Needs review
StatusFileSize
new28.91 KB
new868 bytes

my interdiff is made from the patch in comment 3, and ignoring the last patch

chandeepkhosa’s picture

Issue tags: +Amsterdam2014

updated issue tags to include amsterdam2014

chandeepkhosa’s picture

@derjochenmeyer : I usedgit mv core/themes/seven/css/components/branding.css core/themes/seven/css/components/content-header.css

sqndr’s picture

Status: Needs review » Needs work

You included the sites/default/settings.php file in the patch. Interdiff looks good though ;)

sqndr’s picture

StatusFileSize
new2.36 KB

@ChandeepKhosa: Here's your patch without the settings.php file.

derjochenmeyer’s picture

Status: Needs work » Needs review
StatusFileSize
new59.45 KB

Screenshot of the Seven content header region.

jazio’s picture

Assigned: Unassigned » jazio
jazio’s picture

Assigned: jazio » Unassigned
jazio’s picture

Status: Needs review » Needs work
jazio’s picture

Changed status to trigger the bot.

lewisnyman’s picture

Status: Needs work » Needs review

@jazio.net Did you mean to set it to needs review? The test bot should test it.

jkingsnorth’s picture

Status: Needs review » Needs work
StatusFileSize
new15.07 KB

I've applied the patch and cleared my caches and the region is not displaying correctly, see screenshot.

kiliweb’s picture

We will work on it, and provide a new patch.

LewisNyman queued 15: 2337379-15.patch for re-testing.

lewisnyman’s picture

Issue tags: +Needs reroll
lewisnyman’s picture

Issue tags: -Needs reroll

Yes it looks like we're missing the changes to the info file

roborew’s picture

Hey up, working on a fix to this.

roborew’s picture

fix needed to yml file also, which will need to be added to the patch.

skippednote’s picture

StatusFileSize
new1.82 KB

Change in info file, CSS changes and rename, and updated twig template.

jkingsnorth’s picture

Status: Needs work » Needs review

Setting to needs review then, thanks skippednote

(For information, patch naming conventions can be found here - this makes it easier to match up a downloaded patch with the issue queue again :] )

skippednote’s picture

Fixed missing file issues with the patch.

skippednote’s picture

skippednote’s picture

Resubmitting as the last patch was empty.

mradcliffe’s picture

Just a note that jchin1968, roborew, stephen-cox, kiliweb and lwilliams came up with the same patch as well independently.

nbohn’s picture

I am at the drupalcon sprint amsterdam and try to review this right now.

nathanlawsn’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new24.1 KB
new52.97 KB
new69.56 KB

#33 looks good to me.

content-header.css
content-header.css

seven.info.yml
seven.info.yml

page.html.twig
page.html.twig

The last submitted patch, 29: rename-branding-to-content-header.patch, failed testing.

lewisnyman’s picture

Status: Reviewed & tested by the community » Needs work

There is a test that needs updating. If you view the test results you'll find the test file we need.

kiliweb’s picture

Here is a new patch with the UnitTest updated made with roborew, stephen-cox and jchin1968.

Changes :
1) Renaming branding.css to content-header.css
2) Change the selectors of theses files : page-title.css, tabs.css, content-header.css
3) Inclue content-header.css in the seven.info.yml file
4) Update the test to check the existence of content-header.css instead of branding.css

kiliweb’s picture

Status: Needs work » Needs review
roborew’s picture

Tested patch #42 and it works well.

lewisnyman’s picture

Status: Needs review » Reviewed & tested by the community

This is looking great! Thank you

BarisW’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new4.05 KB

Here's a re-roll

The last submitted patch, 42: drupal-2337379-42.patch, failed testing.

lewisnyman’s picture

Status: Needs review » Needs work
+++ b/core/themes/seven/css/components/page-title.css
@@ -13,3 +13,6 @@
+[dir="rtl"] .content-header h1.page-title {
+  float: right;
+}
\ No newline at end of file

No newline, but I guess this CSS is carried over from an old patch?

BarisW’s picture

Status: Needs work » Needs review
StatusFileSize
new3.88 KB
new327 bytes

Ah, good eyes! Here's a new one.

lewisnyman’s picture

Status: Needs review » Reviewed & tested by the community

Great thanks, I manually test this patch and it still works as expected.

mradcliffe’s picture

Issue summary: View changes

Adjusted issue summary.

lewisnyman’s picture

Issue summary: View changes

To answer the question in the issue summary, we've basically changed all the markup in the whole of core. If we have an change record, it should be a general one for #dreammarkup?

herom’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new400 bytes
new3.66 KB
+++ b/core/themes/seven/css/components/content-header.css
@@ -0,0 +1,11 @@
+[dir="rtl"] .content-header {
+  padding: 20px 20px 0 20px;
+}

This part wasn't needed anymore. The original rule in branding.css was removed in #2343281: Fix RTL issues in content listing page, aka /admin/content.

BarisW’s picture

StatusFileSize
new319.89 KB

Good catch @herom. I've tested your patch. Looks good, and does what it says. Here's a screenshot where the change can be seen.

Screenshot of the newly named content-header region in Seven

+1

lewisnyman’s picture

Status: Needs review » Reviewed & tested by the community

I also manually tested this on RTL as well. RTBC++

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 539ff30 and pushed to 8.0.x. Thanks!

  • alexpott committed 539ff30 on 8.0.x
    Issue #2337379 by skippednote, BarisW, derjochenmeyer, herom,...

Status: Fixed » Closed (fixed)

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