See: #1995272: [Meta] Refactor module CSS files inline with our CSS standards

Remaining tasks

Review the current CSS — What to look for when reviewing CSS

User interface changes

None

API changes

None

Files: 
CommentFileSizeAuthor
#54 rewrite_quickedit_css_inline_2408561-54.patch1.58 KBManinders
#46 rewrite_quickedit_css-2408561-46.patch24.95 KBkostyashupenko
#37 rewrite_quickedit_css-2408561-37.patch24.95 KBLewisNyman
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 116,793 pass(es), 428 fail(s), and 22 exception(s). View
#37 interdiff.txt4.4 KBLewisNyman
#34 interdiff-2408561-31-34.txt4.81 KBntucakovic
#34 2408561-34.patch23.76 KBntucakovic
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,981 pass(es). View
#31 interdiff.txt1.75 KBAleksandar_P
#31 rewrite_quickedit_css-2408561-31.patch23.51 KBAleksandar_P
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,719 pass(es). View
#28 interdiff-2408561-16-28.txt6.97 KBAleksandar_P
#28 rewrite_quickedit_css-2408561-28.patch23.36 KBAleksandar_P
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,834 pass(es). View
#21 rewrite_quickedit_css_inline_2408561-21.patch21.9 KBcchanana
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,797 pass(es). View
#16 rewrite_quickedit_css_inline_2408561-16.patch22.22 KBManjit.Singh
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,763 pass(es). View
#11 rewrite_quickedit_css_inline_2408561-11.patch22.21 KBMathieuSpil
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,418 pass(es). View
#6 rewrite_quickedit_css_inline_2408561-4.patch7.45 KBManinders
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,368 pass(es). View
#3 rewrite-quickedit-css-2408561-3.patch370 bytesManjit.Singh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,951 pass(es). View
#1 quickedit.theme_.css_.txt5.53 KBLewisNyman
#1 quickedit.module.css_.txt2.1 KBLewisNyman
#1 quickedit.icons_.css_.txt1.47 KBLewisNyman

Comments

LewisNyman’s picture

LewisNyman’s picture

Issue summary: View changes
Manjit.Singh’s picture

Status: Active » Needs review
Issue tags: +india, +SprintWeekend2015
FileSize
370 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,951 pass(es). View

@lewis Please verify the patch. There was minor space that needs to remove.

LewisNyman’s picture

Status: Needs review » Needs work

I went over the quickedit CSS files and found more problems to fix, see What to look for when reviewing CSS

  1. .quickedit .icon {
    ...
    .quickedit .icon.icon-end {
    ...
    [dir="rtl"] .quickedit .icon:before {
    

    .quickedit-icon

  2. .quickedit .icon.icon-only {
    

    .quickedit-icon--only

  3. .quickedit .icon.icon-end {
    ...
    [dir="rtl"] .quickedit .icon.icon-end {
    

    .quickedit-icon--end

  4. [dir="rtl"] .quickedit .icon:before {
    ...
    .quickedit button.icon {
    

    Maybe we can remove this selector and move the this font size property up into the .quickedit-icon selector? Let's find out which selector it's overriding

  5. .quickedit .icon-pencil {
    

    .quickedit-icon--pencil

  6. .quickedit .icon-pencil {
    ...
    .quickedit .icon-close:before {
    ...
    .quickedit .icon-close:active:before {
    ...
    .quickedit .icon-throbber:before {
    

    .quickedit-icon--close

  7. /**
     * Images.
     */
    

    Can we move these image properties up into the selectors above? We can delete this comment.

  8. .quickedit .icon-close:before {
    ...
    .quickedit .icon-throbber:before {
    

    .quickedit-icon--throbber

  9. .quickedit .icon-throbber:before {
    ...
    .quickedit .icon-pencil:before {
    

    .quickedit-icon--pencil

  1. .quickedit-validation-errors .messages.error {
    ...
    [dir="rtl"] .quickedit-validation-errors .messages.error {
    

    .quickedit-validation-errors__messages--error

  2. #quickedit_backstage {
    

    This should be a class, with a hyphen instead of an underscore

  3. .quickedit-form .placeholder {
    

    .quickedit-form__placeholder

  4. .quickedit-toolbar-container {
    

    .quickedit-toolbar__container

  5. .quickedit-toolbar-container > .quickedit-toolbar-pointer,
    

    .quickedit-toolbar__pointer

  6. .quickedit-toolbar-container > .quickedit-toolbar-lining {
    

    .quickedit-toolbar__lining

  7. .quickedit-toolgroup.ops {
    

    .quickedit-toolgroup--ops

    I'm not sure what this classes is for, maybe it needs a more description name?

  8. #quickedit-toolbar-fence {
    

    This should be a class

  1. .quickedit-field.quickedit-editable,
    .quickedit-field .quickedit-editable {
    

    We should be able to reduce this selector to .quickedit-editable

  2. .quickedit-field.quickedit-highlighted,
    .quickedit-form.quickedit-highlighted,
    .quickedit-field .quickedit-highlighted {
    

    The same with these, we should be able to just use .quickedit-highlighted

  3. .quickedit-field.quickedit-changed,
    .quickedit-form.quickedit-changed,
    .quickedit-field .quickedit-changed {
    ...
    .quickedit-editing.quickedit-validation-error,
    .quickedit-form.quickedit-validation-error {
    

    The same with these as well

  4. .quickedit-editing.quickedit-editor-is-popup {
    

    .quickedit-editor.is-popup

  5. .quickedit-toolbar-container {
    

    .quickedit-toolbar__container

  6. .quickedit-toolbar-container > .quickedit-toolbar-content {
    

    .quickedit-toolbar__content

  7. .quickedit-toolbar-container > .quickedit-toolbar-pointer {
    

    .quickedit-toolbar__pointer

  8. .quickedit-toolbar-label {
    

    .quickedit-toolbar__label

  9. .quickedit-toolbar {
      font-family: 'Droid sans', 'Lucida Grande', sans-serif;
    }
    

    We are using an odd font-family, is this supposed to match the Seven theme? In that case it should be "Lucida Grande", "Lucida Sans Unicode", "DejaVu Sans", "Lucida Sans", sans-serif

  10. .quickedit-toolbar-entity {
    

    .quickedit-toolbar__entity

Maninders’s picture

Assigned: Unassigned » Maninders
Maninders’s picture

Assigned: Maninders » Unassigned
Status: Needs work » Needs review
FileSize
7.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,368 pass(es). View

I have changed the classes as per #4 suggestions.

RavindraSingh’s picture

@Maninder, When you are making a change in CSS. I believe this reflects somewhere definitely. So this would be good to add screenshot too to show the output after making change.

LewisNyman’s picture

Status: Needs review » Needs work

It looks like we've changed the CSS, but we haven't changed the markup to match the new classes. The CSS will no longer apply.

LewisNyman’s picture

It looks like a lot of the classes are applied in Javascript. For example: /core/modules/quickedit/js/views/EntityToolbarView.js

MathieuSpil’s picture

Assigned: Unassigned » MathieuSpil
MathieuSpil’s picture

Assigned: MathieuSpil » Unassigned
Status: Needs work » Needs review
Issue tags: +JavaScript
FileSize
22.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,418 pass(es). View

1) Should we still be using clearfix?
2) I changed this.show('ops');bythis.show('quickedit-toolgroup--ops'); But I have no clue what it does.
3) Should we create a follow-up ticket for all the html that gets defined in js, or is this intentional?
4) Removed

#quickedit_backstage {
  display: none;
}

Because I don't think this is used anywhere.
5) Created a first patch so all the classes are now smacss'ed, without changing the css-specificity. This way we can really test this patch so we are sure we don't oversee anything. After someone confirmes that this patch looks like it isn't breaking anything. we start refactoring the css further (without changing all the class-names at once)
6) It seems to me that a lot of js is a bit too complex. (More logical classes now show this, so lets setup a ticket for refactoring after this one is finished)
7) I am also not convinced of the --only modifier, can we use --solo or --no-extra or something?

droplet’s picture

+++ b/core/modules/quickedit/css/quickedit.icons.theme.css
@@ -3,22 +3,22 @@
+.quickedit .quickedit-icon {

why don't remove qualified `.quickedit` ?

MathieuSpil’s picture

Yes, we will need to refactor a whole lot of the css itself.
But first I want to have confirmation all the classes are ok before we start refactoring?

LewisNyman’s picture

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

Ok cool, I'm happy with prefixing the icon classes with quickedit as they appear on the frontend of sites.

1) Should we still be using clearfix?

Yeah clearfix is fine.

3) Should we create a follow-up ticket for all the html that gets defined in js, or is this intentional?

Sounds like a follow up

I can't test this patch because it needs a reroll :( Sorry

Manjit.Singh’s picture

Issue tags: -Needs reroll
FileSize
22.22 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,763 pass(es). View

rerolling a patch.

Manjit.Singh’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Needs work

I manually tested the patch and it looks the same as before. I think we can progress with simplifying the CSS now.

  1. +++ b/core/modules/quickedit/css/quickedit.icons.theme.css
    @@ -3,22 +3,22 @@
    -.quickedit .icon {
    +.quickedit .quickedit-icon {
    ...
    -.quickedit .icon.icon-only {
    +.quickedit .quickedit-icon.quickedit-icon--only {
    ...
    -.quickedit .icon.icon-end {
    +.quickedit .quickedit-icon.quickedit-icon--end {
    ...
    -[dir="rtl"] .quickedit .icon.icon-end {
    +[dir="rtl"] .quickedit .quickedit-icon.quickedit-icon--end {
    ...
    -.quickedit .icon:before {
    +.quickedit .quickedit-icon:before {
    
    @@ -31,43 +31,40 @@
    -[dir="rtl"] .quickedit .icon:before {
    +[dir="rtl"] .quickedit .quickedit-icon:before {
    ...
    -.quickedit .icon-end:before {
    +.quickedit .quickedit-icon--end:before {
    ...
    -[dir="rtl"] .quickedit .icon-end:before {
    +[dir="rtl"] .quickedit .quickedit-icon--end:before {
    ...
    -.quickedit button.icon {
    +.quickedit button.quickedit-icon {
    ...
    +.quickedit .quickedit-icon--pencil {
    

    Can we remove all the .quickedit classes from the selectors? Ideally we only want one class per selector. I know that's not possible everywhere.

  2. +++ b/core/modules/quickedit/css/quickedit.module.css
    @@ -104,16 +101,16 @@
    -.quickedit-toolgroup.ops {
    +.quickedit-toolgroup.quickedit-toolgroup--ops {
    ...
    -[dir="rtl"] .quickedit-toolgroup.ops {
    +[dir="rtl"] .quickedit-toolgroup.quickedit-toolgroup--ops {
    

    Quickedit-toolgroup should be removed

  3. +++ b/core/modules/quickedit/css/quickedit.theme.css
    @@ -229,24 +229,24 @@
    -.quickedit-toolbar-container .quickedit-button.action-cancel {
    +.quickedit-toolbar__container .quickedit-button.quickedit-button--cancel {
    

    We can remove quickedit-toolbar__container

  4. +++ b/core/modules/quickedit/css/quickedit.theme.css
    @@ -229,24 +229,24 @@
    +.quickedit-button.quickedit-button--save:hover,
    +.quickedit-button.quickedit-button--save:active {
    ...
    +.quickedit-button.quickedit-button--saving,
    +.quickedit-button.quickedit-button--saving:hover,
    +.quickedit-button.quickedit-button--saving:active {
    

    We we can remove .quickedit-button

LewisNyman’s picture

CSSlint also says there are quite a few properties with 0px instead of 0

cchanana’s picture

Assigned: Unassigned » cchanana
cchanana’s picture

FileSize
21.9 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,797 pass(es). View

Changes has been implemented as per @LewisNyman comment in #18

cchanana’s picture

Assigned: cchanana » Unassigned
Status: Needs work » Needs review
droplet’s picture

There's many CSS code in this way:

.block .block__element
.block.block__element
.block > .block__element
.block .block--modifier

but I think both BEM & SMACSS are trying to avoid these, and convert them into following way as possible as it can be:

.block__element
.block__element
.block__element
.block--modifier

Also, following pattern looks totally wrong:

WORSE:
.quickedit-toolbar__container.quickedit-toolbar__pointer--top > .quickedit-toolbar__pointer

BAD:
.quickedit-toolbar__pointer--top > .quickedit-toolbar__pointer

OK:
.quickedit-toolbar__pointer--top > .quickedit-toolbar__pointer--top

LewisNyman’s picture

Status: Needs review » Needs work

Yeah if we can reduce them all to single selectors in this issue we should try, but only if we are sure we won't cause regressions.

Manjit.Singh’s picture

Yeah if we can reduce them all to single selectors in this issue

@lewis Which all selectors, Is it only related to quickedit or other ?

LewisNyman’s picture

@Manjit.Singh We are only focusing on quickedit markup and CSS in this issue

Aleksandar_P’s picture

Assigned: Unassigned » Aleksandar_P
Aleksandar_P’s picture

Assigned: Aleksandar_P » Unassigned
Status: Needs work » Needs review
FileSize
23.36 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,834 pass(es). View
6.97 KB

Using a patch from comment #16, I have removed unnecessary classes. The ones that stayed, are the one that are needed for overriding default styles.

I have removed two pieces of code from `quickedit.theme.css` due to their unnecessity. Much of the selectors needed additional classes to override these codes that don't style anything, so I eliminated them.

This code was styling nothing but was making `.quickedit-highlighted` impossible to use without `.quickedit-form` in front of it. Every `.quickedit-form` has `.quickedit-highlighted`.

.quickedit-form {
  box-shadow: 0 0 30px 4px #4f4f4f;
  background-color: white;
}

Same problem here. Every button has its more specific BEM class `.quickedit-button--save` and `.quickedit-button--cancel`. It was not possible to style these two elements without `.quickedit-button` before the specific class.

.quickedit-button:hover,
.quickedit-button:active {
  background-color: #c8c8c8;
  border: 1px solid #a0a0a0;
  color: #2e2e2e;
}
LewisNyman’s picture

Status: Needs review » Needs work

Great, nice work here. I manually tested the patch and there are no visual regressions.

One or two things I picked up from the CSSlint tool:

  1. +++ b/core/modules/quickedit/css/quickedit.icons.theme.css
    @@ -3,22 +3,22 @@
    -.quickedit .icon.icon-only {
    +.quickedit-icon--only {
       text-indent: -9999px;
     }
    

    Negative text-indent doesn't work well with RTL. If you use text-indent for image replacement explicitly set direction for that item to ltr.

  2. +++ b/core/modules/quickedit/css/quickedit.icons.theme.css
    @@ -31,43 +31,40 @@
    -.quickedit button.icon {
    +button.quickedit-icon {
    

    This doesn't seem to have an effect, I think normalise CSS handles this kind of inconsistencies.

  3. +++ b/core/modules/quickedit/css/quickedit.module.css
    @@ -85,15 +82,15 @@
    +.quickedit-toolbar__container {
       max-width: 100%;
       position: absolute;
       max-width: 320px;
    

    Duplicate property 'max-width' found.

  4. +++ b/core/modules/quickedit/css/quickedit.theme.css
    @@ -208,8 +206,8 @@
     .quickedit-button[aria-hidden="true"] {
    

    If we switch the attribute selector around with the class it would be faster. So: [aria-hidden="true"].quickedit-button

  5. +++ b/core/modules/quickedit/css/quickedit.theme.css
    @@ -225,28 +223,30 @@
     /* Button with icons. */
     .quickedit-button:hover,
     .quickedit-button:active {
    -  background-color: #c8c8c8;
    -  border: 1px solid #a0a0a0;
    -  color: #2e2e2e;
    +  /*background-color: #c8c8c8;*/
    +  /*border: 1px solid #a0a0a0;*/
    +  /*color: #2e2e2e;*/
     }
    

    Why are these commented out? Can we just remove them?

  6. .quickedit-editable:focus {
      outline: none;
    }
    

    Outlines shouldn't be hidden unless other visual changes are made. If we don't have a good reason for disabling it maybe we could just remove this?

Aleksandar_P’s picture

Assigned: Unassigned » Aleksandar_P
Aleksandar_P’s picture

Status: Needs work » Needs review
FileSize
23.51 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,719 pass(es). View
1.75 KB

I have changed some of the things mentioned above, however

.quickedit-icon--only {
   text-indent: -9999px;
 }

Negative text indent works fine for RTL and positive does not.

.quickedit-editable:focus {
  outline: none;
}

Outline none is necessary because there is some dark blue border which is overridden with

.quickedit-editable {
  box-shadow: 0 0 0 2px #74b7ff;
}

Now, I moved the outline removing to the `quickedit.theme.css` to the same place where the box-shadow is defined.

MathieuSpil’s picture

Assigned: Aleksandar_P » Unassigned

Unassigning for review ;)

ntucakovic’s picture

Assigned: Unassigned » ntucakovic
Status: Needs review » Needs work

So I've just started reviewing the code and found something I'm not sure what to think about.

There are classes prefixed with 'animate-' which is quite generic and might interfere with further module development or some other modules. There also a class 'animate-only-visibility', 'animate-disable-width' etc, which I think shouldn't be called so generic. I think we should prefix with module name before animate because that's not something that should be used outside the scope of the module.

Although, it should be reusable, which I'm not sure how we can achieve. New CSS file with bunch of combinations regarding animation? I don't think so...

Update: Just discussed with couple of folk at Drupalcon, I'll edit .animate to be a modifier to .quickedit-toolgroup because essentially that's how it's used now.

ntucakovic’s picture

Status: Needs work » Needs review
FileSize
23.76 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,981 pass(es). View
4.81 KB

What I've found with latest patch is described above in #33, so it needs a new review. This is my first patch so let me know if I didn't follow some convention properly :) thanks

ntucakovic’s picture

Assigned: ntucakovic » Unassigned

Changed to unassigned for review

MathieuSpil’s picture

Ok, we need feedback on the changes in #31 AND review on #34.

LewisNyman’s picture

Issue tags: +Needs screenshots, +Novice
FileSize
4.4 KB
24.95 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 116,793 pass(es), 428 fail(s), and 22 exception(s). View

Negative text indent works fine for RTL and positive does not.

We should always set direction to LTR when using negative text indent, see the information here: https://github.com/CSSLint/csslint/wiki/disallow-negative-text-indent

Outline none is necessary because there is some dark blue border which is overridden with

I looked at this and it seems that this isn't used? The elements that have this class applied are divs, so I don't think they will ever receive focus.

I ran through the patch, made these changes and reformatted the single line comments to match our standards. We just need a few screenshots to show we haven't broken anything. This is a novice task.

Status: Needs review » Needs work

The last submitted patch, 37: rewrite_quickedit_css-2408561-37.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 37: rewrite_quickedit_css-2408561-37.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 37: rewrite_quickedit_css-2408561-37.patch, failed testing.

Status: Needs work » Needs review

The last submitted patch, 21: rewrite_quickedit_css_inline_2408561-21.patch, failed testing.

vg3095’s picture

Issue tags: +Needs reroll

Patch needs re-roll

error: patch failed: core/modules/quickedit/js/views/FieldDecorationView.js:240
error: core/modules/quickedit/js/views/FieldDecorationView.js: patch does not apply
kostyashupenko’s picture

Manjit.Singh’s picture

Version: 8.0.x-dev » 8.2.x-dev
Status: Needs review » Needs work

It would be move into 8.2.x

magi.yv’s picture

Assigned: Unassigned » magi.yv
magi.yv’s picture

Assigned: magi.yv » Unassigned
magi.yv’s picture

Status: Needs work » Needs review

Patch #46 is working fine on 8.2.x. Can anyone confirm it ?

Manjit.Singh’s picture

So now we need the screenshots (before/after) that Quickedit is working fine after applying the latest patch.

Maninders’s picture

droplet’s picture

  1. +++ b/core/modules/quickedit/js/theme.js
    @@ -37,14 +37,14 @@
    +    html += '<div id="' + settings.id + '" class="quickedit quickedit-toolbar__container clearfix">';
    

    why `.quickedit` here

  2. +++ b/core/modules/quickedit/js/theme.js
    @@ -37,14 +37,14 @@
    +    html += '<div class="quickedit-toolbar quickedit-toolbar__entity quickedit-icon quickedit-icon--pencil clearfix">';
    ...
    +    html += '<div class="quickedit-toolbar quickedit-toolbar__field clearfix" />';
    

    why use `.quickedit-toolbar` & `quickedit-toolbar__*` at same time?

Maninders’s picture

@droplet I just remove the quickedit classes as mentioned in #53.
Please review the patch.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Title: Rewrite quickedit CSS inline with our CSS standards » Rewrite Quick Edit CSS to meet our CSS standards
Status: Needs review » Needs work
Issue tags: +css-novice

#37 is >20K. This is 1.5 K. That makes no sense. It sounds like something went wrong.

emma.maria’s picture

Status: Needs work » Needs review

#54 has lost a lot of what was in #46.
#46 somehow still applies to 8.3.x so it can be reviewed :-)

Wim Leers’s picture

Title: Rewrite Quick Edit CSS to meet our CSS standards » [PP-1] Rewrite Quick Edit CSS to meet our CSS standards
Related issues: +#2828528: Add Quick Edit Functional JS test coverage

Thanks, @emma.maria!

This should be committed after #2828528: Add Quick Edit Functional JS test coverage, to ensure this does not break Quick Edit.

But, the review process can continue in the mean time :) Does this need a review from me (component maintainer), or from a CSS maintainer?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.