Refactor color module CSS inline with our CSS standards

Having a look it.

Removed all CSS ID selectors from the the Color module CSS files. This has an impact on JS logic as well as on Bartik, which implements the color logic.

Instead of simply re-using the ID as a class (e.g.: #placeholder to .placeholder), I prefixed them with color- (.color-placeholder). I'm afraid just using the class is too generic, and might apply styles in contexts they were not intended for.

Brilliant, love the BEM notation. On it.

Hi,

I have also started to work on it, and on the Bartik side wich is linked,
maybe we need to create a Bartik issue for the "preview" part of color module on the theme settings because a lot of work to do here.

Refactored, using is-* for state. There are no JS only classes; they all relate to presentation, so we shouldn't use js-* classes in this particular case.

Status change. Sorry, interdiff and patch in #7 did not take #6 into account.

Sorry, interdiff is incorrect I see. Re-rolling patch from #7, correcting interdiff.

On it.

Updated.

Switching to .js-color-palette is a good idea, but just be aware Bertik adds a small CSS rule for the form, so now uses js-color-palette. It's small, and the standards say we "shouldn't" use js-* classes in CSS, but that doesn't mean there are no, very rare, cases where we can. Here, for example, the basic color component works fine without any additional styling. Only Bartik adds a slight margin and positioning to make it look a little better.

Or we add a double class, .color-palette.js-color-palette ...

Applied the double CSS class approach (.js-color-palette and .color-palette).

Yes, I noticed. I opened a separate issue here: #2470769: Color module lock hooks don't show up correctly. This bug is not related to the class changes. Checking out on 8.0.x reveals the same problem. There's already a patch in the linked issue.

In fact, your first screenshot already shows the bug; it's not correctly showing the lines at the third locked lock. Now that I look closer, the locks are actually too low. A lock should sit between 2 fields, not next to a field.

Here's a screenshot of D7. The difference with your "before" screenshot is quite clear:

Ok, patch doesn't apply anymore; HEAD has moved. Plus, new HEAD introduces another bug (lock icons not showing up anymore).

We need to be careful. Color module and Bartik are being updated elsewhere as well. Latest HEAD broke the lock icons. I fixed it in this patch (check interdiff). Also, the locks were not correctly aligned. I fixed it here as well, instead of opening yet another issue.

For reference, here's the state after applying patch in #18:

The hooks are still incorrect, see #2470769: Color module lock hooks don't show up correctly.

Fixing small syntax issue.

As noted in #15 and #19, the JavaScript has an error, it is reported here #2470769: Color module lock hooks don't show up correctly and the issue already has a patch.

I'll fix the text-indent issue while we're at it.

Here we go again :-)

Ok, HEAD moved. Re-rolling patch.

Ok, re-rolled.

@LewisNyman Please verify the patch.

Patch #36 was indeed missing the changes to the color module preview.js

I could apply patch 29 without any problems so I queued #29 for a re-test.

Please ignore the last part of my previous comment :)

I re-rolled patch #29

Added the interdiff between patch 29 & 42

+++ b/core/modules/color/css/color.admin.css
@@ -27,57 +28,68 @@
+/**
+ * The button also inherits from .link, which hides the background. Use a more
+ * specific selector to overwrite.
+ */
+button.color-palette__lock,
+.color-palette__lock {
...
-[dir="rtl"] #palette .lock {
+[dir="rtl"] .color-palette__lock {

I guess the rtl version needs the more specific selector too.

@alexpott Please verify the patch now.

Thanks for the updated patch @Manjit.Singh
I added an interdiff to show the difference between patch #42 & yours.

To me, this patch looks fine since you added the more specific selector @alexpott asked for in #45.

I noticed we still have to delete this '\ No newline at end of file'

diff --git a/core/modules/color/preview.html b/core/modules/color/preview.html
index e25b7ad..d89edcb 100644
--- a/core/modules/color/preview.html
+++ b/core/modules/color/preview.html
@@ -1,7 +1,7 @@
-<div id="preview">
+<div class="color-preview">
   <div id="text">
     <h2>Lorem ipsum dolor</h2>
     <p>Sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud <a href="#">exercitation ullamco</a> laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
   </div>
   <div id="img"></div>
-</div>
\ No newline at end of file
+</div>

@pjbaert but if you check it manually there is a blank line after </div>.

:) you're right. I checked this manually.

I can't wait to be able to use csslint to test all commits that change CSS.

Committed 1ea7b90 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

+++ b/core/modules/color/css/color.admin.css
@@ -27,57 +28,68 @@
+/**
+ * The button also inherits from .link, which hides the background. Use a more
+ * specific selector to overwrite.
+ */
+button.color-palette__lock,
+.color-palette__lock {
...
-[dir="rtl"] #palette .lock {
+[dir="rtl"] .color-palette__lock {

Hmmm... the rtl variant is still missing for the more specific selector this patch adds.

+++ b/core/modules/color/css/color.admin.css
@@ -104,17 +116,18 @@
-  #palette .lock {
+  button.color-palette__lock,
+  .color-palette__lock {
     float: left; /* LTR */
   }
-  [dir="rtl"] #palette .lock {
+  [dir="rtl"] .color-palette__lock {
     float: right;
   }

This can be removed it is the same as the default styling.

Also the current patch completely messes up the form - it's a regression from head.

Patch

Head

Okay I see from #16 that we fixing a regression from Drupal 7 here already. I think that fixing that here is scope creep but I guess we can let that slide to get this done. So #57 is not an issue but #55 and #56 are because the scope of this issue is to Refactor color module CSS inline with our CSS standards. #55 would be a regression introduced by the patch and #56 is part of the don't have unnecessary duplicate CSS that if it is not part of our standard - it should be.

Can the usability fix introduced by the patches subsequent to #16 by documented on the issue summary. Thanks.

Posting the interdiff of 46 to 58. Looks good.

Adding the followup to fix the hook display to the related issues.

Committed 271766e and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.