Problem/Motivation

The current styles inherited from Core plus how Bartik attempts to provide separate styles for all list components is causing visual upset in Bartik.

Bartik has numerous bug / cleanup issues for various components that are or contain lists.

#2501843: item-list class inherited the wrong margin in RTL
#2541252: Replace the .region-content ul/ol selector with text-formatted to refactor code + fix visual bugs
#2509902: Remove block.css list styles and add the code to other components.
#2468867: Vertical tabs shifted in node type form
#2543928: Remove table ul.link styles in Bartik.
#2509390: tags has a wrong right padding in RTL

If we can add a sensible base style for lists in Bartik, hopefully it will help solve / almost solve all of the issues above.

Proposed resolution

Add base HTML element styling for ol and ul lists in Bartik. Plus account for all of the different list components Bartik has.

I added a solution to help problems with lists in blocks in this issue #2509902: Remove block.css list styles and add the code to other components. that includes base list styles . Maybe this can be used for the global fixing lists issue?

Remaining tasks

Identify the list types in Bartik
Write a patch
Visually test all list components in Bartik
Repeat if bugs, otherwise RTBC

User interface changes

Only changes to fix visual bugs with lists in Bartik.

API changes

none

Data model changes

none.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because we are adding CSS to Core to help fix many other bug issues
Issue priority Not critical because lists in Bartik currently function.
Unfrozen changes Unfrozen because it only changes CSS
Prioritized changes The main goal of this issue is usability of Bartik. We are improving visual bugs and improving the experience of Bartik.
Disruption Non- Disruptive
CommentFileSizeAuthor
#18 add_sensible_base-2548805-18.patch5.45 KBemma.maria
#18 interdiff-15-18.txt1.19 KBemma.maria
#15 add_sensible_base-2548805-15.patch5.26 KBemma.maria
#15 interdiff-12-15.txt1.38 KBemma.maria
#15 tables.png38.44 KBemma.maria
#15 vertical-tabs.png115.28 KBemma.maria
#15 tips-looks-fine.png209.54 KBemma.maria
#15 content-lists-look-the-same.png121.71 KBemma.maria
#15 tags.png39.87 KBemma.maria
#15 item-list-screenshot.png37.14 KBemma.maria
#12 add_sensible_base-2548805-11.patch5.01 KBemma.maria
#12 interdiff-9-11.txt379 bytesemma.maria
#9 add_sensible_base-2548805-9.patch5.01 KBemma.maria
#8 backstop.json_.zip638 bytesemma.maria
#6 interdiff-5-6.txt1.63 KBemma.maria
#6 add_sensible_base-2548805-6.patch5.01 KBemma.maria
#6 add-sensible-link-layout-styles.pdf17.69 MBemma.maria
#6 Screen Shot 2015-08-12 at 16.21.44.png35.21 KBemma.maria
#5 interdiff-4-5.txt806 bytesemma.maria
#5 add_sensible_base_2548805-5.patch4.53 KBemma.maria
#4 interdiff-3-4.txt3.58 KBemma.maria
#4 add_sensible_base-2548805-4.patch4.12 KBemma.maria
#3 add_sensible_base-2548805-3.patch559 bytesemma.maria
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

emma.maria created an issue. See original summary.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
FileSize
add_sensible_base-2548805-3.patch559 bytes

I have searched through Bartik's CSS and there are specific list styles for these components/ regions of the theme.

.field-name-field-tags
.field-type-taxonomy-term-reference
.password-suggestions
header
.block
.item-list
ul.links
.region-content
ol.search-results
.site-footer
.featured-bottom
table ul.links
.tabs

Most of them provide reset styles of padding: 0 or margin: 0 or both and as we have so many visual inconsistencies for lists in Bartik I can justify adding the base styles in for now. I will now go through these components and see what code I can remove that can be provided by HTML element styles.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Title: Add sensible base styles for lists in Bartik. » Add sensible base layout styles for lists in Bartik.
Status: Active » Needs review
FileSize
add_sensible_base-2548805-4.patch4.12 KB
interdiff-3-4.txt3.58 KB

I went through and tidied up the link styles. I removed the .block ul/ol styles (yippee) that were causing so much grief everywhere. I added/removed margin and padding from various components as needed to handle this change.

I kept list layout styles for the regions as they do not follow the rules of menu links, bullet points etc and need overriding layout styles for lists.

I was unable to remove the region-content ul/olstyles. This is because lists within node content need their own class to style them. This is handled in #2541252: Replace the .region-content ul/ol selector with text-formatted to refactor code + fix visual bugs.

Once the region region-content ul/ol selector is removed we can also refactor..

.field-name-field-tags
ol.search-results

As there will not be any other component styles affecting them and the base styles can just be used.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
FileSize
add_sensible_base_2548805-5.patch4.53 KB
interdiff-4-5.txt806 bytes

I found a mistake in the code + found a visual regression regarding breadcrumbs.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
FileSize
Screen Shot 2015-08-12 at 16.21.44.png35.21 KB
add-sensible-link-layout-styles.pdf17.69 MB
add_sensible_base-2548805-6.patch5.01 KB
interdiff-5-6.txt1.63 KB

I did some visual regression testing and fixed a few visual bugs caused by vertical spacing on components that previously had the 0.25em padding bottom on them. Also note the elements that have their horizontal alignment fixed as part of the improvement work in this issue :)
 

^ this has been bothering me since forever.

New patch, visual regression test report and interdiff attached.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Needs work 
+++ b/core/themes/bartik/css/components/breadcrumb.css
@@ -7,3 +7,6 @@
+.region-breadcrumb {
...
+}

Is there a reason why we can't use the .breadcrumb class here instead of the region name? The region name is not reusable elsewhere.

Also we can cut this down to padding: 0 15px 0.25em; with the same effect

@emma.maria Do you have the config you used for the visual regression testing?

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
FileSize
backstop.json_.zip638 bytes

I used the breadcrumb region as the region needs the layout for any block that could be added to it. The breadcrumb could also be moved within another region.

Apologies for the padding declaration formatting I was trying to get tests to pass.

Config file attached.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
FileSize
add_sensible_base-2548805-9.patch5.01 KB

Here's an initial reroll of #6.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Status: Needs work » Needs review
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Needs work

Setting back to needs work back on #7

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Status: Needs work » Needs review
FileSize
interdiff-9-11.txt379 bytes
add_sensible_base-2548805-11.patch5.01 KB

I fixed the padding code issue mentioned in #7.

Also @lewisnyman can you address the feedback in #8 that I left for your question in #7.

I used the breadcrumb region as the region needs the layout for any block that could be added to it. The breadcrumb could also be moved within another region.

Thanks :)

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented

I used the breadcrumb region as the region needs the layout for any block that could be added to it. The breadcrumb could also be moved within another region.

So do we want this styling to apply to the breadcrumb component where ever it is used or do we want it to apply to anything inside of that region? This would make more sense if the region didn't have a silly name.

emma.maria’s picture

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

No the region needs the padding for the grid layout. The breadcrumb block needs no padding styles, it should take on whatever region layout styles are around it. Make sense?

PS. Yes the region name is silly, it's a completely "vanilla" region also in terms of styles and layout, nothing special.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
FileSize
item-list-screenshot.png37.14 KB
tags.png39.87 KB
content-lists-look-the-same.png121.71 KB
tips-looks-fine.png209.54 KB
vertical-tabs.png115.28 KB
tables.png38.44 KB
interdiff-12-15.txt1.38 KB
add_sensible_base-2548805-15.patch5.26 KB

I missed some reset padding and margin styles that also needed to be added to RTL styles. If we are setting a component with a general padding: 0; the RTL needs it too.

Reset styles for some list components are needed as lists have margin and padding out of the box and some components (for eg. no bullet list links) do not need this.

I have also tested the 6 related issues with the latest patch in this comment applied. I can confirm the following:

#2501843: item-list class inherited the wrong margin in RTL - Fixed

#2541252: Replace the .region-content ul/ol selector with text-formatted to refactor code + fix visual bugs - Simplified the patch

We now can do the straightforward class replacement as carried out in the original patch without adding fixes to other parts of the theme.
 

 

https://www.drupal.org/node/2509902 - Fixed

This work has been completed in this issue.

https://www.drupal.org/node/2468867 - Fixed

#2543928: Remove table ul.link styles in Bartik. - currently cannot replicate at HEAD

#2509390: tags has a wrong right padding in RTL - Fixed

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Needs work

Nice, thanks for the screenshots. 

+++ b/core/themes/bartik/css/components/item-list.css
@@ -3,11 +3,20 @@
-  margin: 0;
+  margin: 0; /* LTR */
   padding: 0.2em 0.5em 0 0; /* LTR */
 }
 [dir="rtl"] .item-list ul li {
+  margin: 0;

This is a little confusing because margin: 0 is not LTR specific styling. Why are we adding margin: 0 to the RTL styling? Maybe it needs a comment?

emma.maria’s picture

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

Yeah I agree on the comments. I tried to explain it in #15 amongst the screenshots.

I missed some reset padding and margin styles that also needed to be added to RTL styles. If we are setting a component with a general padding: 0; the RTL needs it too.

Reset styles for some list components are needed as lists have margin and padding out of the box and some components (for eg. no bullet list links) do not need this.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Status: Needs work » Needs review
FileSize
interdiff-15-18.txt1.19 KB
add_sensible_base-2548805-18.patch5.45 KB

I added comments to the RTL declarations that needed explaining.

LewisNyman’s picture

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

Great stuff, thanks.

alexpott’s picture

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

Committed 117a457 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 117a457 on 8.0.x 
    Issue #2548805 by emma.maria, LewisNyman: Add sensible base layout...

Status: Fixed » Closed (fixed)

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