Problem/Motivation

Sub-issue of #1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core

Inline with the CSS cleanup efforts of the HTML5 initiative, using CSSLint at http://csslint.net provides a quick way to code-sniff our css and tweak styles.

Files:modules/ckeditor/ckeditor.admin.css and ckeditor-iframe.css

Proposed resolution

Clean up css in ckeditor module

  1. Copy and paste the the stylesheet(s) below into the css lint tool at http://csslint.net and test.
  2. Fix any warnings or errors the tool finds.
  3. Patch Drupal 8 locally and make sure the css changes have not broken anything visually.
  4. Create patch and upload for the testbot.

Remaining tasks

User interface changes

Should be no difference between before and after.

Before

before screenshot

After

after screenshot

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because coding standards
Issue priority Not critical because it causes no errors
Unfrozen changes Unfrozen because it only changes CSS
CommentFileSizeAuthor
#1 ckeditor-css-cleanup-2470617.patch2.73 KBmajmunbog
#1 ckeditor_d8.png30.18 KBmajmunbog
#1 ckeditor_d8_after-photo.png139.11 KBmajmunbog
#5 ckeditor-article-after(Chrome).png41.28 KBManjit.Singh
#5 ckeditor-article-before(Chrome).png41.43 KBManjit.Singh
#5 ckeditor-IE9-after.png20.75 KBManjit.Singh
#5 ckeditor-IE9-before.png15.87 KBManjit.Singh
#6 ckeditor-css-cleanup-2470617-6.patch2.94 KBMathieuSpil
#11 after.png362.55 KB_nolocation
#11 before.png355.68 KB_nolocation
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

majmunbog’s picture

majmunbog CreditAttribution: majmunbog commented
FileSize
ckeditor-css-cleanup-2470617.patch2.73 KB
ckeditor_d8.png30.18 KB
ckeditor_d8_after-photo.png139.11 KB

Patch and screenshots added.

majmunbog’s picture

majmunbog CreditAttribution: majmunbog commented
Status: Active » Needs review
Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
Issue tags: +frontend, +#css

Seems fine from myside, No issues found in CSSlint. :)

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Assigned: majmunbog » Unassigned
Issue summary: View changes
Issue tags: -#css +CSS, +Needs screenshots

Nice! All we need now are some before/after screenshots to show there are no changes.

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
FileSize
ckeditor-article-after(Chrome).png41.28 KB
ckeditor-article-before(Chrome).png41.43 KB
ckeditor-IE9-after.png20.75 KB
ckeditor-IE9-before.png15.87 KB

Found no issues in Ckeditor :)

MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil commented
Issue tags: -Needs screenshots
FileSize
ckeditor-css-cleanup-2470617-6.patch2.94 KB

Tested the patch manually, it looks great!

Extra: I noticed that since #1872206: Improve CKEditor toolbar configuration accessibility, we are no longer using the .ckeditor-row-controls-class. And because that ticket was fixed for over a year ago. We can assume that this is not being used anymore.

So based upon patch in #1, I removed some unused styling:

.ckeditor-row-controls {
  float: right; /* LTR */
  font-size: 18px;
  width: 40px;
  text-align: right; /* LTR */
}
[dir="rtl"] .ckeditor-row-controls {
  float: left;
  text-align: left;
}
.ckeditor-row-controls a {
  display: inline-block;
  box-sizing: border-box;
  padding: 6px 2px;
  height: 28px;
  width: 20px;
  line-height: 0.9;
  font-weight: bold;
  color: #333;
  text-decoration: none;
}

@Manjit: Be sure to also check this page: /admin/config/content/formats/manage/full_html, there is a lot of the changes that happened there.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Component: CSS » ckeditor.module
Issue tags: +Needs screenshots, +Novice

This is looking good :) I can't see any CSSlint errors, now we just need some screenshots before/after the patch.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented

Specifically, we need before/after screenshots of: /admin/config/content/formats/manage/full_html

_nolocation’s picture

_nolocation CreditAttribution: _nolocation commented
Assigned: Unassigned » _nolocation
Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented

@MathieuSpil everything is fine on /admin/config/content/formats/manage/full_html :) Can you please paste the screenshots where you find some changes due to latest patch.

_nolocation’s picture

_nolocation CreditAttribution: _nolocation commented
FileSize
before.png355.68 KB
after.png362.55 KB

Screenshots taken on OS X Chrome Version 41.0.2272.118
Looks good to me.

_nolocation’s picture

_nolocation CreditAttribution: _nolocation commented
Assigned: _nolocation » Unassigned
YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

@_nolocation thank you for doing the screenshots.

Next time, please embed the screenshots in the issue summary in the UI changes section of the issue summary template (the http://dreditor.org browser plugin helps make that easier). See also https://www.drupal.org/contributor-tasks/add-screenshots

I put the screenshots in the issue summary.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots

Removing the needs screenshot tag. Everything else looks good and @LewisNyman ok'd this earlier.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

  • webchick committed b80d11a on 8.0.x 
    Issue #2470617 by trboslav, MathieuSpil, Manjit.Singh, _nolocation,...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented

Committed and pushed to 8.0.x. Thanks!

Congratulations to trboslav for their first commit mention! :D

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Still haven't had coffee yet. ;)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Thanks for the clean-up! :)

Status: Fixed » Closed (fixed)

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