Problem/Motivation

The shortcut module has CSS that doesn't follow the guidelines set forth by the CSS Cleanup effort (http://drupal.org/node/1089868).

Proposed resolution

Generalize CSS selectors and remove styling where it should be left to the theme to style.

Remaining tasks

None

User interface changes

None. CSS in the Core module is simplified, but does not affect the look of the interface.

API changes

None

Original report by johnvsc

// This is a copy/paste from the CSS Cleanup page
Part of the CSS Cleanup effort: http://drupal.org/node/1089868

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.

Comments

David_Rothstein’s picture

Title: Clean up the CSS for Shortcut module » Split the Shortcut module CSS into separate files for structure/design/etc.

There's already an issue for cleaning up the CSS at #724782: Clean up the shortcut module's CSS.

I'm retitling this to describe what sounds like the main goal of this issue that makes it different from that one.

jyve’s picture

Assigned: Unassigned » jyve
jyve’s picture

Status: Active » Needs review
StatusFileSize
new9.2 KB

Cleaned up the css and renamed the css files to indicate the difference between frontend and backend.
Most of the feedback in #742184: Shortcut.module integration was incorporated into this patch.

To answer the question in #724782: Clean up the shortcut module's CSS: the separation between front and backend seems indeed wrong, but the fact that adding/removing a shortcut can be done from the front- and the backend makes it necessary for most of the css to be in shortcut.theme.css.

jacine’s picture

Title: Split the Shortcut module CSS into separate files for structure/design/etc. » Clean up the CSS for the Shortcut module

I closed #724782: Clean up the shortcut module's CSS, so we can continue to work here.

Also, linking the issue where @sun had done some work per David in the closed issue: #742184-2: Shortcut.module integration

Thanks for getting started on this @jyve!

jyve’s picture

StatusFileSize
new10.63 KB

I had another look at this patch, and with the feedback on other patches, learned that seperation between theme and base.css was needed.

idflood’s picture

StatusFileSize
new10.58 KB

Patch in #5 looks nice and is working well. Here is a little bit more cleanup, maybe too much but it doesn't break anything in firefox and chrome as far as I've tested.

Summary of changes made to the patch in #5:
- span.icon and span.text
- some "ul li" became simply "li"

Status: Needs review » Needs work
Issue tags: -html5, -Front end

The last submitted patch, shortcut-1217038-6.patch, failed testing.

idflood’s picture

Status: Needs work » Needs review

#6: shortcut-1217038-6.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +html5, +Front end

The last submitted patch, shortcut-1217038-6.patch, failed testing.

jyve’s picture

hi idflood, your changes seem to make sence to me!

idflood’s picture

Status: Needs work » Needs review
StatusFileSize
new10.51 KB

Here is a reroll of patch in #6.

jyve’s picture

StatusFileSize
new6.09 KB

Just tested the patch in #11, and looks great.

Since we are ironing out the details, I fixed three missing spaces between properties and values.

idflood’s picture

The last patch is missing the new file, judging the filesize. I think that you need to do something like this:
git add -N modules/*
git diff HEAD > tmp.patch

jyve’s picture

StatusFileSize
new10.51 KB

ow shoop, this should be better.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Tagging as novice for the task of rerolling the Drupal 8.x patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

jyve’s picture

Status: Needs work » Needs review
StatusFileSize
new11.01 KB

Rerolled the patch against the new folder structure, and added @file headers.

xjm’s picture

Issue tags: -Novice

Untagging since it was rerolled. :)

aspilicious’s picture

Status: Needs review » Needs work

Needs reroll

jacine’s picture

Issue tags: +sprint

Tagging for the next sprint.

aspilicious’s picture

Status: Needs work » Needs review
StatusFileSize
new11.58 KB

Updated patch

dcmouyard’s picture

StatusFileSize
new10.74 KB
  • Alphabetized CSS properties according to coding standards.
  • Fixed RTL style sheets.
  • Added /* LTR */ comment on all properties changed in RTL style sheets.
aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/shortcut/shortcut.base-rtl.cssundefined
@@ -0,0 +1,30 @@
+  position: absolute;
+  left: 0;

kLmnoP

L before P

17 days to next Drupal core point release.

+++ b/core/modules/shortcut/shortcut.theme.cssundefined
@@ -0,0 +1,89 @@
+  -webkit-border-radius: 0 5px 5px 0; /* LTR */

We don't need webkit border radius anymore. We decided that in an other issue. Border works fine on every webkit browser these days.

17 days to next Drupal core point release.

dcmouyard’s picture

Status: Needs work » Needs review
StatusFileSize
new10.14 KB
jyve’s picture

Patch in #24 tested, and looks perfect to me.

kenwoodworth’s picture

Status: Needs review » Reviewed & tested by the community

I tested the patch in #24 and it looks good to me.

kenwoodworth’s picture

Issue summary: View changes

Add the CSS cleanup documentation.

catch’s picture

Status: Reviewed & tested by the community » Fixed

This looks like good cleanup. Committed/pushed to 8.x.

Status: Fixed » Closed (fixed)

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

abdul24’s picture

Closed #666698: Add-remove-shortcut link issues as a duplicate of this issue.

abdul24’s picture

Issue summary: View changes

Added issue summary