Problem/Motivation

Currently the region names for footer are:

footer_firstcolumn: 'Footer first column'
  footer_secondcolumn: 'Footer second column'
  footer_thirdcolumn: 'Footer third column'
  footer_fourthcolumn: 'Footer fourth column'
  footer: Footer

It does not make sense to have variants of footer for the first 4 regions and then just plain footer for the last region. Also we should avoid naming regions based on their layout for responsive regions. Also footer needs to fit more in with other region naming conventions where we have used first, second etc.

As discussed here #2398471: Clean up the "footer" component in Bartik in comments 8, 9 and 12.

The new proposed markup and regions for Bartik's footer are as follows....

<div class="site-footer__wrapper">
  <footer class="site-footer">
    {% if page.footer_first or page.footer_second or page.footer_third or page.footer_fourth %}
      <div class="site-footer__top clearfix">
        {{ page.footer_first }}
        {{ page.footer_second }}
        {{ page.footer_third }}
        {{ page.footer_fourth }}
      </div>
    {% endif %}
    {% if page.footer_fifth %}
      <div class="site-footer__bottom role="contentinfo">
        {{ page.footer_fifth }}
      </div>
    {% endif %}
  </footer>
</div>

Markup work is covered in this issue #2398471: Clean up the "footer" component in Bartik but region work was taken out as it is a more delicate topic to be dealt with separately.

Proposed resolution

Replace the existing footer regions with...

footer first
footer second
footer third
footer fourth
footer fifth

Remaining tasks

Write a patch
Write a change record for the name changes, see here for a similar region name changing issue draft.
Review the patch - code review
Review the patch - visual review of Bartik + screenshots
Review the change record

User interface changes

API changes

Test instructions

Apply the patch
Reinstall Drupal 8 on your computer
Then test the new regions and provide screenshots.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because this is part of the cleanup of Bartik. The current region names confuse some and do not make sense now that Bartik is responsive.
Unfrozen changes Unfrozen because it's related to themes. We want Bartik to be a good theme example and this work will contribute to that.
Prioritized changes The main goal of this issue is usability. The column regions are wrongly named as columns do not exist on mobile devices. And we shouldn't have a region called only 'footer' amongst other regions that exist in the footer. The names do not make sense and should have standardised names.
Disruption Disruptive for people who currently use the Bartik theme, but we are keeping all of the regions in the same configuration plus added a change record.
CommentFileSizeAuthor
#50 after footer rename.png32.77 KBemma.maria
#50 before footer rename.png35.03 KBemma.maria
#43 rename-footer-regions-in-bartik-2402639-43.patch11.83 KBlakshminp
#36 Bildschirmfoto 2015-01-19 um 00.19.21.png13.77 KBdernetzjaeger
#36 Bildschirmfoto 2015-01-19 um 00.18.42.png68.35 KBdernetzjaeger
#35 Screen Shot 2015-01-18 at 22.21.36.png49.92 KBemma.maria
#33 problem-with-when_viewing_a-2362435-32.patch.png65.47 KBgippy
#32 rename-bartik-footer-regions-2402639-32.patch11.81 KBlakshminp
#27 rename_bartik_footer_regions-2402639-26.patch5.66 KBPrashant.c
#23 interdiff.txt1.91 KBKatiemouse
#23 2402639-23-css-match-php.patch6.61 KBKatiemouse
#21 2402639-21-rename-footer-regions-bartik.patch6.97 KBrhysmdnz
#19 2402639-19-rename-footer-regions-bartik.patch5.19 KBKatiemouse
#15 d8-rename-footer-regions-bartik-2402639-15.patch5.3 KBnitvirus
#5 d8-rename-footer-regions-bartik-2402639-5.patch5.54 KBnitvirus
#1 rename-bartik-footer-regions-2402639-1.patch4.12 KBkporras07
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kporras07’s picture

kporras07 CreditAttribution: kporras07 commented
FileSize
rename-bartik-footer-regions-2402639-1.patch4.12 KB

Attaching patch with proposed solution.

Regions name changes

Footer first column becomes Footer first
Footer second column becomes Footer second
Footer third column becomes Footer third
Footer fourth column becomes Footer fourth
Footer column becomes Footer fifth

This will also have an impact on any Bartik subthemes, and be most apparent on admin/structure/block, and in the configuration settings for placing individual blocks in regions.

(Change record taken from here and modified)

kporras07’s picture

kporras07 CreditAttribution: kporras07 commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: rename-bartik-footer-regions-2402639-1.patch, failed testing.

nitvirus’s picture

nitvirus CreditAttribution: nitvirus commented
Assigned: Unassigned » nitvirus
nitvirus’s picture

nitvirus CreditAttribution: nitvirus commented
Issue tags: +#dcdelhi
FileSize
d8-rename-footer-regions-bartik-2402639-5.patch5.54 KB

Hi,
I have attached the patch file for renaming the footer section in bartik theme.
files changed:
1. core/themes/bartik/bartik.info.yml
2. core/themes/bartik/css/layout.css
3. core/themes/bartik/templates/page.html.twig

Regions named changed:

Footer first column becomes Footer first
Footer second column becomes Footer second
Footer third column becomes Footer third
Footer fourth column becomes Footer fourth
Footer column becomes Footer fifth

nitvirus’s picture

nitvirus CreditAttribution: nitvirus commented
Status: Needs work » Needs review

Changed the status from needs work to needs review.

Status: Needs review » Needs work

The last submitted patch, 5: d8-rename-footer-regions-bartik-2402639-5.patch, failed testing.

nitvirus’s picture

nitvirus CreditAttribution: nitvirus commented

This patch shouldn't have failed. applying for retesting.

Status: Needs work » Needs review

nitvirus queued 5: d8-rename-footer-regions-bartik-2402639-5.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 5: d8-rename-footer-regions-bartik-2402639-5.patch, failed testing.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue tags: +Needs change record

The patch will fail as the region names for footer have been changed and the tests are looking for the original names.

Also...

+++ b/core/themes/bartik/css/layout.css
@@ -45,10 +45,10 @@ body,
-#footer-wrapper {
+.site-footer__wrapper {
   padding: 35px 0 30px;
 }
-#footer-wrapper .section {
+.site-footer__wrapper .site-footer {
   box-sizing: border-box;
   padding: 0 15px;

This patch should only contain region renaming work in the CSS, templates and info file. Markup around the regions is being worked on in #2398471: Clean up the "footer" component in Bartik.

nitvirus’s picture

nitvirus CreditAttribution: nitvirus commented

Thanks for the reply Emma,

Could you help me a bit, how can this patch be accepted. I suppose I need to remve the css changes from the patch and then upload a new patch.

Thanks for the help.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented

Yes apply your patch in #5 to Core and put back the original selectors that you changed in your first patch, which I highlighted in #11. Then create and upload a new patch from that work.

nitvirus’s picture

nitvirus CreditAttribution: nitvirus commented

Thanksfor the reply emma. Would be uploading the patch

nitvirus’s picture

nitvirus CreditAttribution: nitvirus commented
FileSize
d8-rename-footer-regions-bartik-2402639-15.patch5.3 KB

Uploading the patch again. Keeping my fingers crossed.

Thanks,
nitvirus

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 15: d8-rename-footer-regions-bartik-2402639-15.patch, failed testing.

Katiemouse’s picture

Katiemouse CreditAttribution: Katiemouse commented
Assigned: nitvirus » Katiemouse
Issue tags: +CatalystAcademy

Previous patch by @nitvirus does not apply, so I am going to reroll.

Katiemouse’s picture

Katiemouse CreditAttribution: Katiemouse commented
Assigned: Katiemouse » Unassigned
Status: Needs work » Needs review
FileSize
2402639-19-rename-footer-regions-bartik.patch5.19 KB

Hi this is my rerolled version of the patch, it is the exact same as @nitvirus'. I have made no functional changes :)

Status: Needs review » Needs work

The last submitted patch, 19: 2402639-19-rename-footer-regions-bartik.patch, failed testing.

rhysmdnz’s picture

rhysmdnz CreditAttribution: rhysmdnz commented
Status: Needs work » Needs review
FileSize
2402639-21-rename-footer-regions-bartik.patch6.97 KB

Hi, I updated the tests to use the new region name

Status: Needs review » Needs work

The last submitted patch, 21: 2402639-21-rename-footer-regions-bartik.patch, failed testing.

Katiemouse’s picture

Katiemouse CreditAttribution: Katiemouse commented
Status: Needs work » Needs review
FileSize
2402639-23-css-match-php.patch6.61 KB
interdiff.txt1.91 KB

Hi I have tried to fix some of the errors from the previous patch :)

Status: Needs review » Needs work

The last submitted patch, 23: 2402639-23-css-match-php.patch, failed testing.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented

As we have renamed the regions there will be tests based on the old names that will now fail.
The content so far in the patch is great, we just need to work on getting the tests passing now :)

saki007ster’s picture

saki007ster
he/his
Location Hartford, Connecticut
CreditAttribution: saki007ster commented
Assigned: Unassigned » saki007ster
Issue tags: +#punedrupalgroup #SprintWeekend2015
Prashant.c’s picture

Prashant.c
he/him
Primary language English
Location Dharamshala
CreditAttribution: Prashant.c commented
Assigned: saki007ster » Unassigned
Status: Needs work » Needs review
FileSize
rename_bartik_footer_regions-2402639-26.patch5.66 KB

Adding patch for renaming Bartik footer regions.

saki007ster’s picture

saki007ster
he/his
Location Hartford, Connecticut
CreditAttribution: saki007ster commented

@Prashant.c The patch in comment # 23 does same we need to update the test cases for renaming the footer regions. See @emma.maria 's comment #25 .

piyuesh23’s picture

piyuesh23 CreditAttribution: piyuesh23 commented
Issue tags: -#punedrupalgroup #SprintWeekend2015 +#punedrupalgroup, +#SprintWeekend2015
piyuesh23’s picture

piyuesh23 CreditAttribution: piyuesh23 commented
Issue tags: -#SprintWeekend2015 +SprintWeekend2015
llbbl’s picture

llbbl CreditAttribution: llbbl commented

@saki007ster I don't see any test cases that need to be updated, did you mean to say that we need to add some new ones for renaming the footer regions? The closest one I found was testThemeTableFooter() in core/modules/system/src/Tests/Theme/TableTest.php. Sorry I wanted to help with this one but unsure exactly what needs to be done.

lakshminp’s picture

lakshminp CreditAttribution: lakshminp commented
FileSize
rename-bartik-footer-regions-2402639-32.patch11.81 KB

updated block placements and other CSS parts(layout, typography, colors module support etc).

gippy’s picture

gippy CreditAttribution: gippy commented
FileSize
problem-with-when_viewing_a-2362435-32.patch.png65.47 KB

After applying rename-bartik-footer-regions-2402639-32.patch and clearing the cache, we get a warning:

The block bartik_footer was assigned to the invalid region footer and has been disabled.
The block bartik_powered was assigned to the invalid region footer and has been disabled.

problem with patch 32 leaving blocks in invalid regions

The patch should not disable existing blocks that are left in invalid regions.

gippy’s picture

gippy CreditAttribution: gippy commented
Status: Needs review » Needs work
emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
Screen Shot 2015-01-18 at 22.21.36.png49.92 KB

As the patch contains changes to yml files, I reinstalled Drupal after I applied the patch and it is working fine.

I haven't had a chance to review this thoroughly but I am putting this back to Needs Review, I do not get the errors in #33 and I see this on the Block layouts page....

So the patch does work correctly and I have added test instructions to the issue summary.

Can someone please manually test the patch, check there are no further errors from using the regions after a fresh install and provide screenshots showing the newly named regions in action please??

Also we still need a Change Record to be created for the new names as stated in the "Remaining tasks" section in the issue summary.

dernetzjaeger’s picture

dernetzjaeger CreditAttribution: dernetzjaeger commented
FileSize
Bildschirmfoto 2015-01-19 um 00.18.42.png68.35 KB
Bildschirmfoto 2015-01-19 um 00.19.21.png13.77 KB

I can confirm it works correctly.

lakshminp’s picture

lakshminp CreditAttribution: lakshminp commented

I've created a draft change record here: https://www.drupal.org/node/2409999.
Please review.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record, -Needs manual testing

I tested the patch and it works fine. I also reviewed the change record. I changed to title to "Footer regions in Bartik have been renamed" which feels more accurate and I removed the markup example, as the change record is only about the region names.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Sure, evaluation added.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: rename-bartik-footer-regions-2402639-32.patch, failed testing.

lakshminp’s picture

lakshminp CreditAttribution: lakshminp commented
FileSize
rename-footer-regions-in-bartik-2402639-43.patch11.83 KB

Re-rolling patch with latest dev revision.

LinL’s picture

LinL CreditAttribution: LinL commented
Status: Needs work » Needs review

Setting to Needs review.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs review » Reviewed & tested by the community

Thanks, this patch was knocked out by the #2306407: Remove breadcrumb from page template

I diffed the two patches and they are the same

alexpott’s picture

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

Committed adc801b and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed adc801b on 8.0.x 
    Issue #2402639 by Katiemouse, nitvirus, lakshminp, dernetzjaeger, emma....
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

There are 2 CRs??? I've published the one with the better title - however the other still unpublished has better content. Can someone copy the image over?

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented

Oops this was not spotted as the incorrect one wasn't posted in the issue. I have copied over the footer layout image to the published change record https://www.drupal.org/node/2409999.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
FileSize
before footer rename.png35.03 KB
after footer rename.png32.77 KB

This issue caused a small CSS regression in Core.

Before:


 

After:


 

I have raised a new issue as I also found that another issue also added a small regression, I have combined the work into this issue... #2422975: Bartik footer has CSS regressions..

PLEASE PLEASE PLEASE add screenshots and manually test Bartik before setting these CSS cleanup issues to RTBC.

Status: Fixed » Closed (fixed)

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

a.milkovsky’s picture

a.milkovsky
he/him
Location UTC +2
CreditAttribution: a.milkovsky commented

I also received warning from #33.

The block bartik_footer was assigned to the invalid region footer and [warning]
has been disabled.
The block bartik_powered was assigned to the invalid region footer [warning]
and has been disabled.

Re-saving of blocks layout settings(/admin/structure/block) fixed the warnings issue for me.

It also fixed strange exception "The "field_item:" plugin does not exist." that I got on configuration import.