Rewrite block.admin.css inline with our CSS standards

+++ b/core/modules/block/css/block.admin.css
@@ -46,10 +46,10 @@ a.block-demo-backlink:hover {
-#edit-settings-admin-label label {
+.block-form .form-item-settings-admin-label label {
...
-#edit-settings-admin-label label:after {
+.block-form .form-item-settings-admin-label label:after {

Does it need to be in .block-form?

Hey all, sorry to increase the scope of this issue but it would be nice to fix the underlying problem here instead of just changing the selector.

1. +++ b/core/modules/block/css/block.admin.css
   @@ -46,10 +46,10 @@ a.block-demo-backlink:hover {
    #block-placed {
      background-color: #ffd;
    }
   

   Instead of using #block-placed we can use the .color-warning class to reuse the colors in the Seven theme, and delete this CSS.

2. +++ b/core/modules/block/css/block.admin.css
   @@ -46,10 +46,10 @@ a.block-demo-backlink:hover {
   -#edit-settings-admin-label label {
   +.block-form .form-item-settings-admin-label label {
   ...
   -#edit-settings-admin-label label:after {
   +.block-form .form-item-settings-admin-label label:after {
   

   The reason we have this CSS is because the label element is being abused to just display a some information. If we used the strong element instead of label then we wouldn't need any CSS at all and we could delete this.

I've made changes as per #3. Attached a patch

+++ b/core/modules/block/js/block.admin.js
@@ -79,7 +79,7 @@
-            scrollTop: $('#block-placed').offset().top - $container.offset().top + $container.scrollTop()
+            scrollTop: $('.color-warning').offset().top - $container.offset().top + $container.scrollTop()

+++ b/core/modules/block/src/BlockListBuilder.php
@@ -244,10 +244,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+              'data-element' => '.color-warning',
...
-            $form['blocks'][$entity_id]['#attributes']['id'] = 'block-placed';
+            $form['blocks'][$entity_id]['#attributes']['class'] = 'color-warning';

Ah, ok in this situation the block-placed ID is actually being used for functionality, we should keep this class to use it for functionality, by prefixing it with .js-. We would then have two classes: color-warning for styling and js-block-placed for functionality.

Changing that class has probably broken tests, I would do a search in test files for the block-placed string and replace it with js-block-placed

Rerolling #5

Just figured it out why the tests are breaking. It's because we are changing from label to strong.
The tests use XPATH with label for the selector.

@Lewis @skippednote @mortendk How should we proceed with it now? any suggestions?

Added a patch for js class prefixing.

I have applied #18 patch. After applying patch, I created one custom block named "testing" and assigned it in "Footer fourth" region but found that the custom block is not draggable i.e. not able to re-position it to another region.

Please find the attached screenshot.

@ Kapil Doshi There is a bug in the Block module where a block is not moved to the correct region when the target region is empty. This issue is being resolved in a separate issue: #2177335: Selecting 'Disabled' does not move the block to the disabled region when there are no disabled blocks

The patch in #18 does not include the css lint fixes from #13

@idebr I'm looking into it today.

Time to merge!

Looks fine. Attaching shots

i have tested and found the patch 2422399-23.patch resolves the issue

I ran this through CSSlint using the toolkit and I still get one error:

Line	Column	Message
55	5	Using width with padding-right can sometimes make elements larger than you expect.

Fixes done :
Css Linting
Label replaced with Strong tag
JS ID replacement

Changing label to strong effect almost all labels. Shots attached

@aliyakhan I think that the label has to stay a label for accessibility reasons? Also this change would change all labels in Drupal :\ I think we should leave the element as it is and try and fix this in only CSS

Yeah doesn't make sense to modify the markup. Will update the patch again.

Ok this inludes ID to Class modifications in js and csslint fixes.

@aliyakhan Thanks for working on this!

1. +++ b/core/modules/block/css/block.admin.css
   @@ -43,13 +43,10 @@ a.block-demo-backlink:hover {
   -#block-placed {
   -  background-color: #ffd;
   -}
   

   In #3 @LewisNyman suggested to use the .color-warning class. However, Seven is the only theme that has a color-warning declaration. By removing this, other themes no longer have a visual cue of where the block went. Since this is a usability regression, let's just update the selector instead to the fix the lint error.

2. +++ b/core/modules/block/src/BlockListBuilder.php
   @@ -247,9 +247,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +            $form['blocks'][$entity_id] = array(
   +                '#attributes' => array (
   +                  'class' => array('color-warning', 'js-block-placed')
   +                  )
   +                );
   

   This indentation is incorrect: the Drupal coding standard dictate two spaces for indentation.

At Midcamp sprints, going to work on comments in #34.

I fixed the extra white space mentioned in #34-2. After the bot tests, this still needs more work; I will leave the css/class issues to someone more experienced with that than me as mentioned in #34-1.

It looks like the remaining task is to use a different selector than #block-placed, but keep the background color style. The issue summary should probably be updated with remaining task and probably a beta evaluation.

Disregard these comments, I took a stab at it, but I don't really know what needs to be done next. I'll just stand back, watch, and learn.

Hey Ide

I couldn't found any usability regression with other themes. color-warning class is available.Attached shots

By removing this, other themes no longer have a visual cue of where the block went. Since this is a usability regression, let's just update the selector instead to the fix the lint error.

We don't need to make this change. The colour cue is not required for functionality, as it is still possible to see the block has moved. If colour was required then we would have an accessibility issue :) It will be communicated to admin theme maintainers that modules rely on these utility classes and they need to support them, see Changes to Drupal 8 that affect admin theme maintainers.

Also, I'm generally not fan of issues where we are only fixing CSSLint errors and ignoring other improvements, as the CSSlint errors are only supposed to be there to help us maintain the quality of the CSS. If we avoid problems that we know we have to fix then we are shooting ourselves in the foot a little.

If we agree then I think that the last patch is RTBC. I've added the beta summary evaluation.

Can we just move that visual cue into classy?

We could, but we it would overwriting the JS in Classy, we haven't done that before

@LewisNyman Ah good point, well I don't think the color-warning class is needed (and is not used in this patch). I was just thinking the CSS yellow color moved to classy. I think that color-warning class doesn't need to exist in core or at all.

1. +++ b/core/modules/block/css/block.admin.css
   @@ -43,13 +43,10 @@ a.block-demo-backlink:hover {
   -#block-placed {
   -  background-color: #ffd;
   -}
   

   Not sure about this change, but maybe it can be placed in classy to preserve that color and style?

2. +++ b/core/modules/block/css/block.admin.css
   @@ -43,13 +43,10 @@ a.block-demo-backlink:hover {
   -#edit-settings-admin-label label {
   +.block-form .form-item-settings-admin-label label {
   ...
   -#edit-settings-admin-label label:after {
   +.block-form .form-item-settings-admin-label label:after {
   

   +1 to these changes.

3. +++ b/core/modules/block/css/block.admin.css
   @@ -60,8 +57,7 @@ screen and (min-width: 780px),
   -    width: 75%;
   -    padding-right: 2em;
   +    width: 73%; /* Reducing 2% for whitespace to 100% container */
   

   Need @LewisNyman's opinion on this change. Seems suspect to me.

4. +++ b/core/modules/block/js/block.admin.js
   @@ -79,7 +79,7 @@
   -            scrollTop: $('#block-placed').offset().top - $container.offset().top + $container.scrollTop()
   +            scrollTop: $('.js-block-placed').offset().top - $container.offset().top + $container.scrollTop()
   

   I'm happy to have this change in core.

5. +++ b/core/modules/block/src/BlockListBuilder.php
   @@ -247,9 +247,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +            $form['blocks'][$entity_id] = array(
   +              '#attributes' => array (
   +              'class' => array('color-warning', 'js-block-placed')
   +              )
   +            );
              }
   -
   

   The formatting on this is off by a bunch. Use the same syntax, it will save some grief.

   $form['blocks'][$entity_id]['#attributes']['class'][] = 'color-warning';
   $form['blocks'][$entity_id]['#attributes']['class'][] = 'js-placed-block';

   And color-warning class IMO isn't needed.

I think that color-warning class doesn't need to exist in core or at all.

There is justification for it under #2336141: Create reusable color classes and Changes to Drupal 8 that affect admin theme maintainers

I don't mind moving the reuseable classes to Classy, as long as they are loaded as part of the Seven theme.

The remaining tasks are:

+++ b/core/modules/block/css/block.admin.css
@@ -60,8 +57,7 @@ screen and (min-width: 780px),
-    width: 75%;
-    padding-right: 2em;
+    width: 73%; /* Reducing 2% for whitespace to 100% container */

Remove this change from this patch and leave the layout stuff to #2335531: Replace block placement page layout CSS with reusable layout classes

+++ b/core/modules/block/src/BlockListBuilder.php
@@ -247,9 +247,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+            $form['blocks'][$entity_id] = array(
+              '#attributes' => array (
+              'class' => array('color-warning', 'js-block-placed')
+              )
+            );
           }
-

The formatting on this is off by a bunch. Use the same syntax, it will save some grief.

Re-rolled and changed as #45 suggested.

+++ b/core/modules/block/src/BlockListBuilder.php
@@ -247,9 +247,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-            $form['blocks'][$entity_id]['#attributes']['id'] = 'block-placed';
+            $form['blocks'][$entity_id]['#attributes']['class'][] = 'color-warning';

There is no .color-warning class in core/stark, so adding this class only really helps the seven theme... kinda(different style background color #fdf8ed).

I'd rather not do a form_alter in classy just to get this color class and it's very very close in color/intent. So I suggest we leave it here in the patch, but please add a .color-warning { background-color: #ffd; } class to classy theme to preserve this.

But looking at classy, there is ironically no CSS in it, I'm assuming yet... So maybe we just leave .color-warning {} in core/modules/block/css/block.admin.css for now with that class on it?

Everything else looks great.

hmm, I'm not sure. In my mind its perfectly fine to have classes in Classy that don't have styling in Classy. We're trying remove CSS that we have to override in themes, see the motivation in #2336141: Create reusable color classes

There are situations where modules want to define colours that are consistent of the Seven theme, in some situations we've just copied over the colour values that are defined in the Seven theme, in some situations the modules just make up their own colours and it looks inconsistent.

I think adding reusable color classes into Stark or Classy is a bigger discussion for another issue, because it's totally fine not to show any color if it's not required for functionality (and it shoudn't if used correctly)

I think that maybe this is the wrong issue to start moving classes or CSS into Classy in this issue, if we manage to remove custom CSS here then it's an improvement. Setting back to needs review.

I ran this through CSSlint using the toolkit and I still get one error:

[16:06:14] 1 error found in /Applications/MAMP/htdocs/d8/core/modules/block/css/block.admin.css
[16:06:14] [L61:C5] Using width with padding-right can sometimes make elements larger than you expect. Don't use width or height when using padding or border. (box-model)

But according to the comment #45 this is to be left for #2335531: Replace block placement page layout CSS with reusable layout classes .

Nice, I've updated the issue summary to include the colour change.

Committed e82c6f2 and pushed to 8.0.x. Thanks!

thanks for adding the beta evaluation to the issue summary.