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.
| Comment | File | Size | Author |
|---|---|---|---|
| #56 | content-header.png | 319.89 KB | BarisW |
| #55 | drupal-2337379-55.patch | 3.66 KB | herom |
| #55 | interdiff-2337379-51-55.txt | 400 bytes | herom |
| #51 | interdiff-48-51.txt | 327 bytes | BarisW |
| #51 | drupal-2337379-51.patch | 3.88 KB | BarisW |
Comments
Comment #1
derjochenmeyer commentedHere's a patch.
Comment #2
lewisnymanGreat thanks! As we're changing everything, do you think we could switch from an id to a class? Kill all the IDs!
Comment #3
derjochenmeyer commentedSure, if its just killing that single ID :) Here's an updated patch.
Comment #4
lewisnymanThanks! Here's a screenshot of the content header region looking the same :-)
Comment #5
alexpottSo then branding.css needs to be renamed or merged to another css file
Comment #6
lewisnymanDoh of course, we need to rename it content-header.css
Comment #7
Palashvijay4O commentedSubmitting a patch .
Comment #8
Palashvijay4O commentedComment #9
derjochenmeyer commentedHow 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
Comment #10
chandeepkhosa commented..
Comment #11
chandeepkhosa commentedmy interdiff is made from the patch in comment 3, and ignoring the last patch
Comment #12
chandeepkhosa commentedupdated issue tags to include amsterdam2014
Comment #13
chandeepkhosa commented@derjochenmeyer : I used
git mv core/themes/seven/css/components/branding.css core/themes/seven/css/components/content-header.cssComment #14
sqndr commentedYou included the
sites/default/settings.phpfile in the patch. Interdiff looks good though ;)Comment #15
sqndr commented@ChandeepKhosa: Here's your patch without the
settings.phpfile.Comment #16
derjochenmeyer commentedScreenshot of the Seven content header region.
Comment #17
jazio commentedComment #18
jazio commentedComment #19
jazio commentedComment #20
jazio commentedChanged status to trigger the bot.
Comment #21
lewisnyman@jazio.net Did you mean to set it to needs review? The test bot should test it.
Comment #22
jkingsnorth commentedI've applied the patch and cleared my caches and the region is not displaying correctly, see screenshot.
Comment #23
kiliweb commentedWe will work on it, and provide a new patch.
Comment #25
lewisnymanComment #26
lewisnymanYes it looks like we're missing the changes to the info file
Comment #27
roborew commentedHey up, working on a fix to this.
Comment #28
roborew commentedfix needed to yml file also, which will need to be added to the patch.
Comment #29
skippednote commentedChange in info file, CSS changes and rename, and updated twig template.
Comment #30
jkingsnorth commentedSetting 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 :] )
Comment #31
skippednote commentedFixed missing file issues with the patch.
Comment #32
skippednote commentedComment #33
skippednote commentedResubmitting as the last patch was empty.
Comment #34
mradcliffeJust a note that jchin1968, roborew, stephen-cox, kiliweb and lwilliams came up with the same patch as well independently.
Comment #35
nbohn commentedI am at the drupalcon sprint amsterdam and try to review this right now.
Comment #36
nathanlawsn commented#33 looks good to me.
content-header.css

seven.info.yml

page.html.twig

Comment #38
lewisnymanThere is a test that needs updating. If you view the test results you'll find the test file we need.
Comment #42
kiliweb commentedHere 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
Comment #43
kiliweb commentedComment #44
roborew commentedTested patch #42 and it works well.
Comment #46
lewisnymanThis is looking great! Thank you
Comment #48
BarisW commentedHere's a re-roll
Comment #50
lewisnymanNo newline, but I guess this CSS is carried over from an old patch?
Comment #51
BarisW commentedAh, good eyes! Here's a new one.
Comment #52
lewisnymanGreat thanks, I manually test this patch and it still works as expected.
Comment #53
mradcliffeAdjusted issue summary.
Comment #54
lewisnymanTo 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?
Comment #55
herom commentedThis 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.
Comment #56
BarisW commentedGood catch @herom. I've tested your patch. Looks good, and does what it says. Here's a screenshot where the change can be seen.
+1
Comment #57
lewisnymanI also manually tested this on RTL as well. RTBC++
Comment #58
alexpottCommitted 539ff30 and pushed to 8.0.x. Thanks!