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

CommentFileSizeAuthor
#75 interdiff_74-75.txt13 KBadityasingh
#75 2408561-75.patch35.99 KBadityasingh
#74 rewrite_quickedit_css_inline_2408561-74.patch23.63 KBanushrikumari
#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
#37 interdiff.txt4.4 KBlewisnyman
#34 interdiff-2408561-31-34.txt4.81 KBntucakovic
#34 2408561-34.patch23.76 KBntucakovic
#31 interdiff.txt1.75 KBAleksandar_P
#31 rewrite_quickedit_css-2408561-31.patch23.51 KBAleksandar_P
#28 interdiff-2408561-16-28.txt6.97 KBAleksandar_P
#28 rewrite_quickedit_css-2408561-28.patch23.36 KBAleksandar_P
#21 rewrite_quickedit_css_inline_2408561-21.patch21.9 KBcchanana
#16 rewrite_quickedit_css_inline_2408561-16.patch22.22 KBmanjit.singh
#11 rewrite_quickedit_css_inline_2408561-11.patch22.21 KBmathieuspil
#6 rewrite_quickedit_css_inline_2408561-4.patch7.45 KBmaninders
#3 rewrite-quickedit-css-2408561-3.patch370 bytesmanjit.singh
#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
he/him
commented
StatusFileSize
new quickedit.icons_.css_.txt1.47 KB
new quickedit.module.css_.txt2.1 KB
new quickedit.theme_.css_.txt5.53 KB
lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented
Status: Active » Needs review
Issue tags: +india, +SprintWeekend2015
StatusFileSize
new rewrite-quickedit-css-2408561-3.patch370 bytes

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

lewisnyman’s picture

lewisnyman
he/him
commented
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

maninders commented
Assigned: Unassigned » maninders
maninders’s picture

maninders commented
Assigned: maninders » Unassigned
Status: Needs work » Needs review
StatusFileSize
new rewrite_quickedit_css_inline_2408561-4.patch7.45 KB

I have changed the classes as per #4 suggestions.

RavindraSingh’s picture

RavindraSingh commented

@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

lewisnyman
he/him
commented
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

lewisnyman
he/him
commented

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

mathieuspil’s picture

mathieuspil commented
Assigned: Unassigned » mathieuspil
mathieuspil’s picture

mathieuspil commented
Assigned: mathieuspil » Unassigned
Status: Needs work » Needs review
Issue tags: +JavaScript
StatusFileSize
new rewrite_quickedit_css_inline_2408561-11.patch22.21 KB

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

droplet commented 
+++ 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

mathieuspil commented

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

lewisnyman
he/him
commented
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

lewisnyman’s picture

lewisnyman
he/him
commented

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

manjit.singh
Primary language Hindi
Location Chandigarh
commented
Issue tags: -Needs reroll
StatusFileSize
new rewrite_quickedit_css_inline_2408561-16.patch22.22 KB

rerolling a patch.

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented
Status: Needs work » Needs review
lewisnyman’s picture

lewisnyman
he/him
commented
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

lewisnyman
he/him
commented

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

cchanana’s picture

cchanana commented
Assigned: Unassigned » cchanana
cchanana’s picture

cchanana commented
StatusFileSize
new rewrite_quickedit_css_inline_2408561-21.patch21.9 KB

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

cchanana’s picture

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

droplet commented

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

lewisnyman
he/him
commented
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

manjit.singh
Primary language Hindi
Location Chandigarh
commented

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

lewisnyman
he/him
commented

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

Aleksandar_P’s picture

Aleksandar_P commented
Assigned: Unassigned » Aleksandar_P
Aleksandar_P’s picture

Aleksandar_P commented
Assigned: Aleksandar_P » Unassigned
Status: Needs work » Needs review
StatusFileSize
new rewrite_quickedit_css-2408561-28.patch23.36 KB
new interdiff-2408561-16-28.txt6.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

lewisnyman
he/him
commented
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

Aleksandar_P commented
Assigned: Unassigned » Aleksandar_P
Aleksandar_P’s picture

Aleksandar_P commented
Status: Needs work » Needs review
StatusFileSize
new rewrite_quickedit_css-2408561-31.patch23.51 KB
new interdiff.txt1.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

mathieuspil commented
Assigned: Aleksandar_P » Unassigned

Unassigning for review ;)

ntucakovic’s picture

ntucakovic commented
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

ntucakovic commented
Status: Needs work » Needs review
StatusFileSize
new 2408561-34.patch23.76 KB
new interdiff-2408561-31-34.txt4.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

ntucakovic commented
Assigned: ntucakovic » Unassigned

Changed to unassigned for review

mathieuspil’s picture

mathieuspil commented

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

lewisnyman’s picture

lewisnyman
he/him
commented
Issue tags: +Needs screenshots, +Novice
StatusFileSize
new interdiff.txt4.4 KB
new rewrite_quickedit_css-2408561-37.patch24.95 KB

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

alvar0hurtad0 queued 37: rewrite_quickedit_css-2408561-37.patch for re-testing.

Status: Needs review » Needs work

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

Status: Needs work » Needs review

LewisNyman queued 37: rewrite_quickedit_css-2408561-37.patch for re-testing.

Status: Needs review » Needs work

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

Status: Needs work » Needs review

snehi queued 37: rewrite_quickedit_css-2408561-37.patch for re-testing.

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

vg3095’s picture

vg3095 commented
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

kostyashupenko
Primary language English
Location Omsk
commented
Issue tags: -Needs reroll
StatusFileSize
new rewrite_quickedit_css-2408561-46.patch24.95 KB

Reroll of #37

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented
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

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

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

magi.yv commented
Status: Needs work » Needs review

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

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented

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

maninders’s picture

maninders commented
droplet’s picture

droplet commented 
  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

maninders commented
StatusFileSize
new rewrite_quickedit_css_inline_2408561-54.patch1.58 KB

@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

wim leers
Location Ghent 🇧🇪🇪🇺
commented
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

emma.maria
she/her
commented
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

wim leers
Location Ghent 🇧🇪🇪🇺
commented
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.

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

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

Andrusha’s picture

Andrusha commented
Assigned: Unassigned » Andrusha
Issue tags: +Moldcamp2017
Andrusha’s picture

Andrusha commented
Assigned: Andrusha » Unassigned

Version: 8.5.x-dev » 8.6.x-dev

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

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
commented
Title: [PP-1] Rewrite Quick Edit CSS to meet our CSS standards » Rewrite Quick Edit CSS to meet our CSS standards

#2828528: Add Quick Edit Functional JS test coverage is in.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nessthehero’s picture

nessthehero
he/him
commented

Good afternoon.

I wanted to chime in on this as a fly on the wall, non-contributor, without creating a separate ticket as this one seems the most appropriate and still in progress.

Some feedback about the quickedit css. Could some kind of namespace be added to the .icon class names?

At my company we do a couple of Drupal 8 sites and we also frequently use Icomoon. Icomoon generates some CSS with .icon as the chosen classname, with .icon-{NAME} as each icon's name. The default CSS that Icomoon provides causes some wonky issues with the quickedit panel that appears when inline editing a text or WYSIWYG field.

Obviously I could just change it on my end to reduce clashing, but I feel like .icon is just a generic class name that having it as part of the application seems asking for conflict. Adding a generic namespace like .qe- to the beginning of each class would probably be sufficient.

Again, just some 2 cents. I have a feeling .icon is an existing class you are tying into the functionality of and might be a bigger change than what I'm asking.

Thank you for your time.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
commented
Status: Needs review » Needs work
Issue tags: -JavaScript +JavaScript, +Needs reroll

Some good, thoughtful work in this thread! It would be great to see this get picked up again and have these updates in Quickedit, as well as the broader parent issue #1995272: [Meta] Refactor module CSS files inline with our CSS standards

The patch in #46 no longer applies, (no surprise after 4 years!)
and does need the changes from #54 merged in.

anushrikumari’s picture

anushrikumari commented
Assigned: Unassigned » anushrikumari
atul4drupal’s picture

atul4drupal commented

@anushrikumari We appreciate your effort in helping resolve the issues and making Drupal experience better. We also need to be more concerned about other contributors by not blocking the issue by assigning it to our self, as this practice is generally discouraged @here and even if you are assigning, it helps to mention by when a response is expected from you.

At this point I see you have 2 issues assigned to yourself: this one and the other is 3181778 both tagged as novice and for re-roll.

We preferably should avoid such assignment of issues.

anushrikumari’s picture

anushrikumari commented
Assigned: anushrikumari » Unassigned
Status: Needs work » Needs review
StatusFileSize
new rewrite_quickedit_css_inline_2408561-74.patch23.63 KB

@atul4drupal I've created the patch but was getting failure, that's why it was delayed. Thanks for the suggestion I'll try to avoid that.

adityasingh’s picture

adityasingh commented
Issue tags: -Needs reroll
StatusFileSize
new 2408561-75.patch35.99 KB
new interdiff_74-75.txt13 KB

Fixed #74 Custom Commands Failed. Kindly review the patch.

Status: Needs review » Needs work

The last submitted patch, 75: 2408561-75.patch, failed testing. View results

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

spokje’s picture

spokje
Primary language Dutch
commented
Project: Drupal core » Quick Edit
Version: 9.4.x-dev » 1.0.x-dev
Component: quickedit.module » Code

Due to Quickedit being moved out of Drupal Core and into a Contrib Module, moving this issue to the Contrib Module queue.

amin.ankit’s picture

amin.ankit
Primary language English
Location Gujarat (Vadodara)
commented
Assigned: Unassigned » amin.ankit

Hi, I'll work on this issue.

Thanks,

amin.ankit’s picture

amin.ankit
Primary language English
Location Gujarat (Vadodara)
commented
Assigned: amin.ankit » Unassigned
ravi kant’s picture

ravi kant commented
Status: Needs work » Needs review

I got messages "This module has deprecated" during enabling QuickEdit module.
Are we still contributing this module?