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
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 |
Comment | File | Size | Author |
---|---|---|---|
#18 | add_sensible_base-2548805-18.patch | 5.45 KB | emma.maria |
| |||
#18 | interdiff-15-18.txt | 1.19 KB | emma.maria |
#15 | add_sensible_base-2548805-15.patch | 5.26 KB | emma.maria |
| |||
#15 | interdiff-12-15.txt | 1.38 KB | emma.maria |
#15 | tables.png | 38.44 KB | emma.maria |
Comments
Comment #2
emma.mariaComment #3
emma.mariaI 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
ormargin: 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.Comment #4
emma.mariaI 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/ol
styles. 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..
As there will not be any other component styles affecting them and the base styles can just be used.
Comment #5
emma.mariaI found a mistake in the code + found a visual regression regarding breadcrumbs.
Comment #6
emma.mariaI 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.
Comment #7
LewisNymanIs 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?
Comment #8
emma.mariaI 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.
Comment #9
emma.mariaHere's an initial reroll of #6.
Comment #10
emma.mariaComment #11
LewisNymanSetting back to needs work back on #7
Comment #12
emma.mariaI 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.
Thanks :)
Comment #13
LewisNymanSo 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.
Comment #14
emma.mariaNo 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.
Comment #15
emma.mariaI 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
Comment #16
LewisNymanNice, thanks for the screenshots.
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?
Comment #17
emma.mariaYeah I agree on the comments. I tried to explain it in #15 amongst the screenshots.
Comment #18
emma.mariaI added comments to the RTL declarations that needed explaining.
Comment #19
LewisNymanGreat stuff, thanks.
Comment #20
alexpottCommitted 117a457 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.