Problem/Motivation

Using ID selectors as styling hooks are bad practice according to our CSS coding standards. We should use classes.

As mentioned in #2321121-13: Remove #console we have some id-s in page.html.twig such as #branding, #content, #help, #footer and some styling applied in the related css files to #branding and #help.

Proposed resolution

We should switch from id-s to class-es both in twig and in the related css files.

Branding is managed in #2337379: Rename 'branding' to 'content-header' but we have three other id-s

Remaining tasks

Wait until we get #2307533: Insufficient space at page bottom in.

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions Complete
Manually test the patch Novice Instructions
Embed before and after screenshots in the issue summary Novice Instructions

User interface changes

None

API changes

None

CommentFileSizeAuthor
#17 Remove-id-selectors-2346791-17.patch1.75 KBrpayanm
#15 Remove-id-selectors-2346791-15.patch1.74 KBrpayanm
#12 After-patch(Firefox).png22.16 KBPalashvijay4O
#12 Before-patch(Firefox).png21.48 KBPalashvijay4O
#12 After-patch(Chrome).png25.37 KBPalashvijay4O
#12 Before-patch(Chrome).png22.57 KBPalashvijay4O
#7 removed-id-selectors-from-page-templates-of-seven-2346791.7.patch1.42 KBPalashvijay4O
#5 removed-id-selectors-from-page-templates-of-seven-2346791.5.patch1.43 KBPalashvijay4O
#4 Remove-id-selectors-2346791-4.patch1.41 KBrpayanm

Comments

thamas’s picture

thamas
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes
Issue tags: +CSS
Related issues: +#2337379: Rename 'branding' to 'content-header', +#2321121: Remove #console
thamas’s picture

thamas
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes
Related issues: +#2307533: Insufficient space at page bottom
rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Assigned: Unassigned » rpayanm
rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Active » Needs review
StatusFileSize
new Remove-id-selectors-2346791-4.patch1.41 KB
Palashvijay4O’s picture

Palashvijay4O commented
StatusFileSize
new removed-id-selectors-from-page-templates-of-seven-2346791.5.patch1.43 KB

submitting a patch .

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented

@Palashvijay4
Here:

+++ b/core/themes/seven/templates/page.html.twig
@@ -80,11 +80,11 @@
+    <main class="content" class="clearfix" role="main">

and here:

+++ b/core/themes/seven/templates/page.html.twig
@@ -96,7 +96,7 @@
+      <footer class="footer" role="contentinfo" class="layout-column">

There is two instance of class.

Palashvijay4O’s picture

Palashvijay4O commented
StatusFileSize
new removed-id-selectors-from-page-templates-of-seven-2346791.7.patch1.42 KB

Some minor changes as shown to me by @rpayanm . Thank you

thamas’s picture

thamas
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes
Issue tags: +needs screenshots
thamas’s picture

thamas
Primary language Hungarian
Location Hungary
commented
Issue tags: -needs screenshots +Needs screenshots
Palashvijay4O’s picture

Palashvijay4O commented

@thamas : Do we need to post the screenshot of home page before and after we apply the patch ?

sqndr’s picture

sqndr commented

@Palashvijay4: Yes. That would be great. Please also tell which browser the screenshots are from, and make sure to test in multiple browsers. Screenshots from one browser is fine though.

Palashvijay4O’s picture

Palashvijay4O commented
StatusFileSize
new Before-patch(Chrome).png22.57 KB
new After-patch(Chrome).png25.37 KB
new Before-patch(Firefox).png21.48 KB
new After-patch(Firefox).png22.16 KB

Screenshots as taken in Chrome and Firefox respectively.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots

I made a quick sanity check to make sure we didn't break anything. Everything seems good!

alexpott’s picture

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

Are we sure we want to change #content to .content? This will break #2307533: Insufficient space at page bottom

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new Remove-id-selectors-2346791-15.patch1.74 KB

@alexpott here I fix that ;)

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work

Ah yes, unfortunately we are still using the .content class in blocks and comments. I guess we should change .content into .page-content so it's more specific.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new Remove-id-selectors-2346791-17.patch1.75 KB

@LewisNyman here the patch :)

lewisnyman’s picture

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

Excellent thanks. I manually tested the patch just to make sure.

alexpott’s picture

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

Committed 17461de and pushed to 8.0.x. Thanks!

  • alexpott committed 17461de on 8.0.x 
    Issue #2346791 by rpayanm, Palashvijay4 | thamas: Remove id selectors...

Status: Fixed » Closed (fixed)

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

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Assigned: rpayanm » Unassigned