Part of CSS Cleanup Initiative

Overview of Goals

  1. Make it easy to remove unwanted design assumptions in the theme layer, while maintaining critical functionality (such as functional JavaScript widgets).
  2. Prevent uneeded administrative styles from loading on the front end.
  3. Give modules the ability to include a generic design implementation with their module, without burdening themers.
  4. Make CSS and related markup more efficient and less intrusive to improve the themer experience.

The CSS Clean-up Process

Use the following guidelines when writing patches for the core issues listed below.

  1. Put CSS is in the appropriate file: CSS should be moved to separate files, using the following
    name spacing conventions based on their purpose:
    module.base.css
    Should hold structural and behavior related styling. CSS should be coded against the Stark theme. The absolute bare minimum CSS required for the module to function should go here. If there is no CSS required, this file should be omitted.
    module.theme.css
    Should hold generic design-related styles that could be used with Stark and other themes. It's where all design assumptions like backgrounds, borders, colors, fonts, margins, padding, etc, would go.
    module.admin.css
    Should hold styles that are only applicable to administrative pages.

    To see an example of this in practice, look at Drupal's system module.

  2. Remove Assumptions: Styles that make too many assumptions, introduce superflous margins, padding and add things like font settings are not necessary and don't belong in core module CSS files. In cases where core themes depend on these properties, they should be moved to the CSS stylesheet of the respective theme.
  3. Reduce Selector Specificity: CSS code that resides in modules should be written in a way that's easily overridable in the theme layer. To improve the Themer Experience and make core CSS more efficient, CSS selectors should be made as general and short as possible. For example:
    • Use .style {} over div.style {} where possible.
    • Use .module .style {} over div.module div.somenestedelement .style where possible.
  4. Don't use IDs in selectors: Use of ID's in core CSS selectors requires more specificity in the theme layer, making it harder and more annoying to deal with. It makes achieveing consistency in complex design implementations much harder than it needs to be. We need to stop making life hard for theme developers.
  5. Don't be afraid to change markup: There's lots of overlap between using proper and semantic markup and doing CSS right. If you come across a case where CSS is being applied where using a more semantic elements would solve the problem, then change the markup in your patch to make it right. For more information, see the Drupal 8 Markup Gate rules.
  6. Start with Stark and cross-browser test.
    1. "Design" markup and CSS for the Stark theme.
    2. If applicable, adapt the styles to match the core themes afterward.
    3. Finally, test the changes in all supported browsers and ensure no regressions are introduced.
Files: 
CommentFileSizeAuthor
#18 1217032-dashboard-css-6.patch10.58 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 34,807 pass(es).
[ View ]
#16 dashboard-cleanup-1216980-16.patch9.96 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 34,565 pass(es).
[ View ]
#15 dashboard-cleanup-1216980-15.patch10.01 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 34,568 pass(es).
[ View ]
#14 dashboard-cleanup-1216980-14.patch9.56 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 34,576 pass(es).
[ View ]
#13 1216980-12-dashboard-css-cleanup.patch9.3 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 34,568 pass(es).
[ View ]
#8 cleanup_1216980-8.patch1.28 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 33,971 pass(es).
[ View ]
#1 cleanup-1216980-2.patch7.46 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in cleanup-1216980-2.patch.
[ View ]

Comments

JamesK’s picture

Project:Drupal core» Dashboard
Issue summary:View changes

Fixed link to initiative page.

cosmicdreams’s picture

Project:Dashboard» Drupal core
StatusFileSize
new7.46 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in cleanup-1216980-2.patch.
[ View ]

Here's my first shot at this patch. In the end there was only a tiny bit of css needed to make dashboard work as most of it is controlled by jQuery UI.

Also, I changed some of the hardcoded ids to classes. I wanted to convert all of the ids to classes but I couldn't change the #dashboard ID. When I did the dashboard broke.

cosmicdreams’s picture

Status:Active» Needs review
cosmicdreams’s picture

I think there is an argument to be made here that the dashboard.base.css is too sparse. As the dashboard's usability is removed if you only use dashboard.base.css and any theme would exclude the dashboard.theme.css would have to implement many of the same styles to get to a working state.

If the instructions above are read literally, the dashboard.base.css should be as sparse as possible. That is the aim of this patch.

Status:Needs review» Needs work

The last submitted patch, cleanup-1216980-2.patch, failed testing.

cosmicdreams’s picture

After having a conversation with Jacine it's clear that I'll need to rework this patch. Admin specific css should be targetted at dashbaord.admin.css. In this case that's a lot of the styles that are in dashboard.theme.css after this patch is applied.

So, this is a good starting point but more work is needed. I'll likely have more time to look at this later in the week.

jyve’s picture

I just had a quick look at your patch, so here is some feedback that you could incorporate in your updated patch:

The separation between base.css and theme.css seems ok to me, although most of theme.css content should go to admin.css, as you mentioned. In fact, since dashboard is an admin tool I would think that a theme.css is not required here?

Maybe a minor thing we could fix while we are at it is the use of underscores in the dasboard id's. I did not find a convention against that, but I've never seen underscores in other Drupal modules, so we should avoid it here too.

Good luck with the patch!

cosmicdreams’s picture

Assigned:Unassigned» cosmicdreams

I'll try to get this done before the next html 5 sprint starts.

cosmicdreams’s picture

StatusFileSize
new1.28 KB
PASSED: [[SimpleTest]]: [MySQL] 33,971 pass(es).
[ View ]

Here's a revised patch that admittedly doesn't cover all of the issues we care about in this patch (doesn't cover rtl, but I don't see how we can)

It doesn't seem that the module is specifically calling the rtl.css for this module. Can someone explain what needs to be done in order to have rtl support?

Also, how does the patch look?

cosmicdreams’s picture

Status:Needs work» Needs review
jyve’s picture

@cosmicdreams: just some info without looking at the patch:

Drupal automatically picks up -rtl.css files when needed. That means that if you have a dashboard.theme.css, it will automatically look for dashboard.theme-rtl.css when browsing the site in a RTL-language. That dashboard.theme-rtl.css file will then be added after dashboard.theme.css.

skottler’s picture

This looks good to me.

aspilicious’s picture

Status:Needs review» Needs work
+++ b/core/modules/dashboard/dashboard.moduleundefined
@@ -588,7 +588,8 @@ function dashboard_update() {
+  drupal_add_css(drupal_get_path('module', 'dashboard') . '/dashboard.admin.css');
+  drupal_add_css(drupal_get_path('module', 'dashboard') . '/dashboard.theme.css');
   return '<div id="dashboard" class="clearfix">' . $element['#children'] . '</div>';

This doesn't make sense if we are not renaming the original css files.

-22 days to next Drupal core point release.

cosmicdreams’s picture

StatusFileSize
new9.3 KB
PASSED: [[SimpleTest]]: [MySQL] 34,568 pass(es).
[ View ]

Working on this again. This is before I properly segregate the styles into .admin.css and .theme.css

cosmicdreams’s picture

Status:Needs work» Needs review
StatusFileSize
new9.56 KB
PASSED: [[SimpleTest]]: [MySQL] 34,576 pass(es).
[ View ]

This patch refactors dashboard.base.css into two files and eliminates dashboard.theme.css. No CSS has been removed from original implementation yet.

I've tested this with the dashboard.admin.css turned off and it is technically functional but it is quite awkward with the proper paddings and dotted boarders present.

Putting into needs review to test my patch only. Will attempt to prune the css used even further.

cosmicdreams’s picture

StatusFileSize
new10.01 KB
PASSED: [[SimpleTest]]: [MySQL] 34,568 pass(es).
[ View ]

In the last patch, I forgot to refactor the rtl styles. Fixed that with this patch.

cosmicdreams’s picture

StatusFileSize
new9.96 KB
PASSED: [[SimpleTest]]: [MySQL] 34,565 pass(es).
[ View ]

last patch did not have proper endlines on two files. Fixing...

cosmicdreams’s picture

I've been thinking about making a concerned effort to reduce the amount of css used here (specifically targeting the browser specific properties, and other styles that in testing can be removed without degradation of appearance). But I want to test my assumptions can someone review this and let me know if the patch is good to go now, or if a further css cleanup is necessary?

aspilicious’s picture

StatusFileSize
new10.58 KB
PASSED: [[SimpleTest]]: [MySQL] 34,807 pass(es).
[ View ]

"module.admin.css
Should hold styles that are only applicable to administrative pages."

Everything from the dashboard module should be part of admin.css, we don't need base.css in this case.

ps: I still think this module is going away soon, so we shouldn't waste much time on this one.
ps2: wrong issue number in the patch name

jyve’s picture

@aspilicious: it is not because something is only visible in the backend that everything should be in .admin.css. base.admin.css is making sure it works. .admin.css adds styles to that. I'm quite sure this is what Jacine once pointed out.

So I would tend to agree with the patch from #16.

Jacine’s picture

It looks like the only clean up that is going on here is moving selectors into the right file. I'm fine with that because I think this module really needs to be removed from core.

As far as which files to use, since this is strictly for an administrative UI, the styles belong in .admin.css. I know it's confusing for some of these newer modules, but it's different from say, contextual links because those are displayed both on the front and backend. Also things like tabledrag styles, and some of the other stuff in system.base.css can be implemented by modules anywhere, which also makes it "special." That's not going to happen for this module, and we decided early on not to put that kind of energy (splitting admin in the .base and .theme) early on because it's just not worth the effort it would require this time around.

RobLoach’s picture

Project:Drupal core» Dashboard
Version:8.x-dev» 6.x-2.x-dev
Component:dashboard.module» Code
Status:Needs review» Active
cosmicdreams’s picture

Assigned:cosmicdreams» Unassigned

Unassigned issue so I can focus on D8 initiatives.

cosmicdreams’s picture

Issue summary:View changes

Updated issue summary.