Replace block placement page layout CSS with reusable layout classes

Removed the custom CSS and implemented the reusable classes

Hi @LewisNyman

Thanks for the feedback.
I processed you remarks from #3 in this patch.

1. Fixed typo in classname: .three-quarters should be .three-quarter
2. The html of the sidebar: We should keep the .block-list-secondary-class inside the twig for its border. And we should also keep the quarter-class for the width properties. The html is now:

<div class="layout-column quarter block-list-secondary">
    {{ form.place_blocks }}
</div>

3. Fixed weird whitespace in/after media-query:

@media
screen and (max-width: 1020px) {

is now

@media screen and (max-width: 1020px) {

4. Removed the extra use of the.layout-container-class. Otherwise we are nesting .layout-container inside .layout-container

5.

Let's keep it for now and try and figure out how we can remove it in another issue.

@Lewis: I don't think we should keep this for another issue, now the page looks weird because the sidebar now receives a padding-left of 10px (because it is the second layout-container in it's parent) But the .block-list-secondary-class gives it a border-left... Which is a bad combination (weird whitespace, see screenshot...)

I suggest we don't try to restyle this page but rewrite some css so there is no visual change, and it still works with the new layout classes?

I think this ticket should be postponed after we decide on #2471791: Improve the CSS layout framework for Drupal's admin interface or we should already finish point 5 of #7. Because this will stay an issue after the new generic classes for backend pages.

Ok, so all TODO's can now be found in #7.

Setting to needs review to the patch can be properly tested before anyone starts working further on this.

Added a extra div so the border can be applied to the inner div.
I am also not sure if the .entity-meta is being used anywhere else.
If not all border-specifications can also be deleted out of entity-meta.css:

  .entity-meta {
  background-color: #ececec;
  border-bottom: 0;
  border-left: 1px solid #bfbfbf;
  border-right: 1px solid #bfbfbf;
  border-top: 0;
  box-shadow: inset 0 0 5px rgba(0, 0, 0, .15);
  margin-top: 0;
  padding-top: 0;
}

The changes made by @Manjit.Singh look correct.
Triggering the testbot.

Because the new layout-classes use padding as a way for separation, we can not use the border-left and the padding on the same element, that was why I added a extra div.
The solution of Manjit now has visual regression that looks like this (patch #21):

@Lewis: It would be better indeed without the extra div, but we can only apply a border on the elements that are inside the .block-list-secondary-class. And because one of them uses margin as a way to position it, we can't apply the border on that. (Or we should refactor a whole bunch of stuff.)
Maybe we can add a .block-list-secondary--inner-class, but that seems a bit silly to me.

So thanks for the tips on setting up for no more whitespace!
Assuming the patch #21 was a wrong approach because of visual regression (But not really sure), I reworked patch #19 so it no longer contains the wrong whitespace.

Alright! Setting back to needs work

Based upon last patch, i used the block-list-secondary__inner-class instead of a div.

After this ticket, the following ticket can continue: #2473213: Refactor Block CSS inline with our CSS standards.

Hey guys,

My name is Vijay from DrupalCon LA! I just applied the latest patch from #26 to my local and we still see 3 lint errors.

Line	Column	Message
49	1	Don't use IDs in selectors.
52	1	Don't use IDs in selectors.
55	1	Don't use IDs in selectors.

Can we just convert the IDs to classes?

Thanks,
Vijay

-- Removing an accidentally added patch --

Screenshots W.R.T the fixes I added at #30

Screenshots W.R.T the fixes I added at #30

Hey guys,

Spoke with Lewis and he explained me that the linting errors will be handled as a part of a different issue. So now I worked on updating the files as per #28.
Please review.

Thanks,
Vijay

Thanks everyone for your work on this issue including the screenshots.

+++ b/core/modules/block/css/block.admin.css
@@ -58,25 +61,6 @@ a.block-demo-backlink:hover {
-    float: left; /* LTR */
...
-    float: right; /* LTR */
...
-  [dir="rtl"] .block-list-secondary {
-    float: left;
-  }

+++ b/core/modules/system/css/system.admin.css
@@ -368,3 +368,17 @@ small .admin-link:after {
+    padding-right: 0;

We've got some LTR and RTL code being removed, but only one direction of padding being added. We need to have manual testing in RTL as well that's documented in the summary with before-and-after screenshots to ensure there is no RTL regression.

There some changes in width of table and "Place Blocks". Please check before and after screenshots.

Alright!

So thanks @manjit for the work on the screenshots.
The change in widths are caused by the use of the layout-classes that use a different amount of padding instead of using the custom padding previously defined.
The screenshots also show that all visual changes are correct. and there is no RTL-regression on the blocks.

@vijaykumar.info99
Thanks for your work on this! But I do have some feedback for you:
You should try to name your patch the same way as the last uploaded patch of the ticket. (So instead of 2335531_33.patch It should have been named replace-block-placement-page-layout-css-2335531-33.patch) This way it is easier for people working on this ticket to download your patch and apply it on their local install etc..
You should also consider adding an interdiff file so everyone up-to-date with this ticket can read the interdiff so they can see very easily what you changed since last patch.
(I added one for clarity's sake, so everyone can see you indeed incorperated Lewis his feedback in #28)

@Lewis:
Tested the latest patch too and everything looked fine indeed.

Off-topic:
In the screenshots of Manjit, you can see a rtl-regression on the menu above the blocks (too much margin-right in comparison with the LTR-screenshots)
This is the case on both the before and the after-screenshots, so is there a already a ticket about this?

@MathieuSpil

regarding offtopic, It would be good if you can highlight that RTL margin in comparison with the LTR in screenshots.

+++ b/core/modules/system/css/system.admin.css
@@ -368,3 +368,17 @@ small .admin-link:after {
+/**
+ * The vertical toolbar mode gets triggered for narrow screens, which throws off
+ * the intent of media queries written for the viewport width. When the vertical
+ * toolbar is on, we need to suppress layout for the original media width + the
+ * toolbar width (240px). In this case, 240px + 780px.
+ */
+@media screen and (max-width: 1020px) {
+  .toolbar-vertical.toolbar-tray-open .layout-column {
+    float: none;
+    width: auto;
+    padding-right: 0;
+  }
+}

I think this code means that we can remove the following code from block.admin.css

/**
 * The vertical toolbar mode gets triggered for narrow screens, which throws off
 * the intent of media queries written for the viewport width. When the vertical
 * toolbar is on, we need to suppress layout for the original media width + the
 * toolbar width (240px). In this case, 240px + 780px.
 */
@media
screen and (max-width: 1020px) {

  .toolbar-vertical.toolbar-tray-open .block-list-primary,
  .toolbar-vertical.toolbar-tray-open .block-list-secondary {
    float: none;
    width: auto;
    padding-right: 0;
  }
}

+++ b/core/modules/system/css/system.admin.css
@@ -368,3 +368,17 @@ small .admin-link:after {
+/**
+ * The vertical toolbar mode gets triggered for narrow screens, which throws off
+ * the intent of media queries written for the viewport width. When the vertical
+ * toolbar is on, we need to suppress layout for the original media width + the
+ * toolbar width (240px). In this case, 240px + 780px.
+ */
+@media screen and (max-width: 1020px) {
+  .toolbar-vertical.toolbar-tray-open .layout-column {
+    float: none;
+    width: auto;
+    padding-right: 0;
+  }
+}

I think this code means that we can remove the following code from block.admin.css

/**
 * The vertical toolbar mode gets triggered for narrow screens, which throws off
 * the intent of media queries written for the viewport width. When the vertical
 * toolbar is on, we need to suppress layout for the original media width + the
 * toolbar width (240px). In this case, 240px + 780px.
 */
@media
screen and (max-width: 1020px) {

  .toolbar-vertical.toolbar-tray-open .block-list-primary,
  .toolbar-vertical.toolbar-tray-open .block-list-secondary {
    float: none;
    width: auto;
    padding-right: 0;
  }
}

you're right @alexpott.
Since we moved this code to system.admin.css, we can remove this part from block.admin.css

The changes suggested by @alexpott are correctly processed in #45. This can go back to RTBC.

+++ b/core/modules/system/css/system.admin.css
@@ -368,3 +368,17 @@ small .admin-link:after {
+    padding-right: 0;

.layout-column has no padding-right, so this line has no effect. Instead, it should be padding-left to target .layout-column + .layout-column in system.admin.css:29 with an additional override for RTL for [dir="rtl"] .layout-column + .layout-column in system.admin.css:32

Hi @idebr, I think you are taking this ticket one step too far.

This ticket is mainly about implementing the layout-classes on the admin/structure/block page.
But this feedback can be posted in a follow-up ticket or a ticket dedicated to the layout-classes, or admin links-specific.

Can someone point to the correct ticket for feedback on this?

This can likely be closed as a duplicate after #2512456: Implement the new block layout design to emphasize the primary interaction of placing a block, unless someone things we should still genericize this

I think we can work on this issue. As #2512456: Implement the new block layout design to emphasize the primary interaction of placing a block is fixed now.