Problem/Motivation

Bartik's code needs to meet current Drupal coding standards.

Proposed resolution

This issue takes a specific section of Bartik's code, it acts as a "component" issue and improves that section of code with minimal impact on the rest of Bartik's codebase.

This issue aims to clean up and properly format the CSS and templates files without breaking Bartik visually.

This issue primarily looks at the css/components/dropbutton.component.css file.

Work that needs to be included in patches for this issue are fully outlined in the META issue #1342054: [META] Overhaul Bartik's code..

The work needs to be crossed off the list below as completed or stated why they were not applicable to this issue in the comments below, to make sure we cover everything.

Also very helpful! Noting the list items in your comment with the patch to show what parts you added to the patch.

Create a patch containing the potential following work:

Code cleanup work

  1. Check each selector in the CSS file (associated with the particular issue) is in use within core right now.
    If not...
    a) Check to see if the classes in core have been changed and correct them (for e.g. I found this in this issue ).
    or
    b) Remove that CSS completely from the CSS file.
  2. a) Check the CSS selectors are not being replicated in other stylesheets in Bartik.
    b) Check the CSS properties are not being overridden by other stylesheets in Bartik.
    If a) move all of the properties to the selector in the stylesheet that you think most appropriate for the component you are dealing with.
    If a) and b) also remove the CSS properties and values being overridden within that ruleset.
  3. If you find CSS for a component which seems out of place in the file it is currently in move it to the one you think is the correct one.
  4. If a selector appears to be too long and/or too specific, check if the selector can be simplified. for eg. .something .something .something { } being modified to .something .something { }.
  5. Check that RTL styles exist when needed and are formatted as per the guidelines. (for e.g. we found that RTL styles are broken on certain pages in this issue, so fix anything you see missing/incorrect in the CSS file.
  6. If you think the contents of the CSS file could be further broken down into more components CSS files, or grouped together with other existing CSS files to form one component do it. The initial SMACSS issue may not of been perfect, guidelines on CSS file organisation for Drupal 8 can be found here.
  7. Check the markup from the templates that all of the classes are used as selectors in the CSS files. If not remove them. See an example issue here for this.

Code formatting work

  1. Add a File comment to the top of the stylesheet - see here for guidelines.
  2. Check any other comments are formatted correctly - see here for guidelines.
  3. Check Whitespace is being used correctly, this includes indentations and line breaks - see here for guidelines.
  4. Check the formatting of rulesets, properties and media queries are correct - see here for guidelines.
  5. As mentioned above, check existing RTL styles are formatted correctly - see here for guidelines.

Remaining tasks

  • Assess the code applicable to this patch and figure out what work in the lists above need to be included in the patch.
  • Cross out work tasks that do not apply to this issue.
  • Write a patch with as much work as you want to include, upload and comment what you included
  • Review the patch - code review and visual changes
  • Upload screenshots to show nothing/something is broken on the frontend

User interface changes

None, we are cleaning up CSS and markup in templates. The use of Bartik's UI and design will stay the same.

API changes

n/a

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is a code clean up overhaul of a theme.
Unfrozen changes Unfrozen because it only refactors CSS and templates, no changes to UI or APIs
Prioritized changes The main goal of this issue is usability and performance. We want Bartik code to be up to date and something to be proud of.
Disruption No disruption it's only refactoring code not changing how to use the theme
Files: 
CommentFileSizeAuthor
#15 bartik-clean-up-dropbutton-2398467-15.patch4.34 KBlduerig
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,943 pass(es).
[ View ]
#8 2398467-8.patch4.86 KBlduerig
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,231 pass(es).
[ View ]

Comments

emma.maria’s picture

Title:Clean up "dropbutton" CSS in Bartik» Clean up "dropbutton" component in Bartik
emma.maria’s picture

monobasic’s picture

Assigned:Unassigned» monobasic

I'll take this issue as a starter - Drupal Global Sprint Weekend, Zurich.

monobasic’s picture

Assigned:monobasic» Unassigned

.. too much needed know how involved I just realize - back to unassigned

lduerig’s picture

I'll take this one.

lduerig’s picture

Assigned:Unassigned» lduerig
lduerig’s picture

This issue is difficult or impossible to fix properly as long as core dropbutton.css is in it's current state. e.g. line 36 in Bartik's CSS file uses !important which is obviously bad, but hard to get around nicely when in core/misc/dropbutton/dropbutton.css , we have:

.js td .dropbutton-widget {
position: absolute;
}

Suggestions? There are so many cases of over-specificity in core CSS that I think that should be fixed, first...

lduerig’s picture

StatusFileSize
new4.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,231 pass(es).
[ View ]

This is a pretty heavy-handed patch. .js made much of the CSS from core over-specific, I replaced that with .dropbutton-wrapper so it is at least vaguely related to dropbutton.

lduerig’s picture

Status:Active» Needs review
emma.maria’s picture

Thanks for the patch @lduerig!
There is an issue for the core dropbuttons here #1899236: [meta] Rewrite dropbutton into splitbutton.

LewisNyman’s picture

Status:Needs review» Needs work

Thanks for the patch!

I replaced that with .dropbutton-wrapper so it is at least vaguely related to dropbutton.

I think we used the .js class for a reason though, because dropbutton hides some values, we want this styling to only kick in when there is Javascript, otherwise it looks bad for people who don't have JS.

LewisNyman’s picture

Thanks for the patch!

I replaced that with .dropbutton-wrapper so it is at least vaguely related to dropbutton.

I think we used the .js class for a reason though, because dropbutton hides some values, we want this styling to only kick in when there is Javascript, otherwise it looks bad for people who don't have JS.

lduerig’s picture

I can put the .js class back, but in some cases it didn't look particularly good with js disabled, anyway:
https://www.dropbox.com/s/z4a1097ajtfyl5h/Screenshot%202015-03-06%2016.1...

Is that different now in D8 than D7?

Also, does anyone know what the .dropbutton-single class was for? I can't find any usages of it.

lduerig’s picture

StatusFileSize
new4.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,943 pass(es).
[ View ]

Here's a new patch.

lduerig’s picture

Status:Needs work» Needs review
lduerig’s picture

I found .dropbutton-single, the patch seems to still work as previously. However, another instance where this doesn't look great without javascript.

https://www.dropbox.com/s/w2kejngsyijdfpp/Screenshot%202015-03-06%2020.5...