Suggested commit message:

git commit -m 'Issue #2398467 by lduerig, tresti88, Eski, LewisNyman, emma.maria: Clean up "dropbutton" component in Bartik'

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] Clean up templates and CSS.

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
Support from Acquia helps fund testing for Drupal Acquia logo

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

FileSize
4.86 KB

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

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

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...

Vidushi Mehta’s picture

subscribing...

rteijeiro’s picture

Status: Needs review » Needs work

Back to needs work. Also it needs some feedback from @emma.maria or @LewisNyman to continue the work.

LewisNyman’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
15.41 KB
11.44 KB

Looking at the buttons without JS, I think they are identical to how they look before the patch.

This is how I see dropbutton and dropbutton-single with the patch:


Eski’s picture

In order to further review this, I have applied the latest patch and taken screenshots of a couple instances with and without Javascript enabled.

Publishing with Javascript
Only local images are allowed.

Publishing without Javascript
Only local images are allowed.

Operations with Javascript
Only local images are allowed.

Operations without Javascript
Only local images are allowed.

As you can see, there's a little tidying up to do. So I'm setting this to "Needs Work".

Eski’s picture

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

Status: Needs work » Needs review

The screenshots in #21 are of the dropbuttons in Seven and not Bartik.
You will need to enable Bartik as an admin theme and take some screenshots.

Thanks! :)

tresti88’s picture

Content list page before patch screenshots:

Content list page after patch screenshots:

tresti88’s picture

'Add' content before patch screenshots:

'Add' content after patch screenshots:

emma.maria’s picture

Assigned: lduerig » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Setting this to RTBC. The code is much improved and I can confirm along with #24 and #25 that there are no visual regressions.

Suggested commit message:
git commit -m 'Issue #2398467 by lduerig, tresti88, Eski, LewisNyman, emma.maria: Clean up "dropbutton" component in Bartik'

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

CSS is not frozen in beta. Committed 6ba0114 and pushed to 8.0.x. Thanks!

  • alexpott committed 6ba0114 on 8.0.x
    Issue #2398467 by lduerig, tresti88, Eski, LewisNyman, emma.maria: Clean...

Status: Fixed » Closed (fixed)

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