Improve code comment review on mobile

[Gusted]

• Fix placement of avatar image, this was not placed in the comment-header-left and add CSS to cover the limiting of width+height of avatar for code-review comment on "Files changed" page. This fixes the big noticeable avatar issue.
• Apply margin-bottom to the "next" button, so it's consistent with the "previous" button.
• Make sure the "next"/"previous" start at flex-start on mobile and not off-screen at flex-end. As well force them to have flex: 1 so they won't overflow on x-asis. This also requires the width: 100% for the .ui.buttons div.
• Resolves Avatar Fills Width On Small Screen Resolution in Code Comment #20074

Before

After

[silverwind]

I'd say avatar should be left side, otherwise the "speech bubble" effect is pointless.

[Gusted]

I'd say avatar should be left side, otherwise the "speech bubble" effect is pointless.

Never noticed it was a speech bubble.

[Gusted]

Would be an option to remove the "speech bubble icon"?, it's the only comment form that still has this, all other ones are already in the header.

[silverwind]

speech bubble icon

You mean removing the triangle? I think we want to keep these as the speech bubble effect is still present during regular issue comments.

[Gusted]

af142d3 keeps the speech bubble effect as well fix the overflowing "previous/next" buttons. "After" screenshot also updated.

[lunny]

CI failure is related

[silverwind]

Ideally avatar should move into empty space on the left, but I'm afraid that's not easily possible with that table layout in place.

[silverwind]

Try this patch:

diff --git a/web_src/less/_review.less b/web_src/less/_review.less
index 52f10a4daa..c2f85fff62 100644
--- a/web_src/less/_review.less
+++ b/web_src/less/_review.less
@@ -47,8 +47,21 @@
     margin-bottom: .5em;
   }
 }
 
+tr.add-comment {
+  position: relative;
+  left: -90px;
+}
+
+tr.add-comment .comment-code-cloud {
+  width: calc(100% + 90px);
+}
+
+tr.add-comment .comment-code-cloud .comment-container {
+  width: calc(100% - 50px);
+}
+
 .show-outdated,
 .hide-outdated {
   &:extend(.unselectable);
   display: block !important;
@@ -75,20 +88,20 @@
     }
 
     .ui.buttons {
       width: 100%;
+      margin: 0 !important;
 
       .button {
         flex: 1;
       }
     }
   }
 
   .comments .comment {
-    margin: 0;
+    padding: 0;
 
     @media @mediaSm {
-      padding: 0;
       display: flex;
 
       .comment-header-right.actions .ui.basic.label {
         display: none;

[Gusted]

Looks good for mobile, but... for larger screens this looks like a full-width hack 😅 I'm not really sure if we want that.

[silverwind]

I don't mind it as much even on desktop, but yes, I can probably increase right side margin a bit.

[Gusted]

I don't mind it as much even on desktop, but yes, I can probably increase right side margin a bit.

If we break such "user are used to this" it should be minimal, so adding right-side margin(on non-mobile) is fine with me.

[Gusted]

And it's breaking on split diff

[silverwind]

This should be better (and simpler). Full width on mobile only, on desktop, align comment left side with line numbers:

diff --git a/web_src/less/_review.less b/web_src/less/_review.less
index 52f10a4daa..93c7ecab81 100644
--- a/web_src/less/_review.less
+++ b/web_src/less/_review.less
@@ -61,10 +61,13 @@
 .comment-code-cloud {
   padding: .5rem !important;
   position: relative;
   margin: 0 auto;
+  left: -56px;

   @media @mediaSm {
+    left: -90px;
+    width: calc(100% + 80px);
     padding: .75rem !important;

     .code-comment-buttons {
       margin: .5rem 0 .25rem !important;
@@ -75,20 +78,20 @@
     }

     .ui.buttons {
       width: 100%;
+      margin: 0 !important;

       .button {
         flex: 1;
       }
     }
   }

   .comments .comment {
-    margin: 0;
+    padding: 0;

     @media @mediaSm {
-      padding: 0;
       display: flex;

       .comment-header-right.actions .ui.basic.label {
         display: none;

[silverwind]

Indeed, split diff is a problem because line numbers are not as wide.

[Gusted]

Eh.... Try simulating 375x667

Pull page:

File page:

[silverwind]

I give up, there is no clean way via CSS. the <td> for line numbers would need to be removed from HTML rendering to be able to efficiently use that space.

Pushed a few small margin/padding changes that are non-destructive.

[silverwind]

Actually, I think I found something that works.

[silverwind]

New version uses colspan and display: none to use all available space. It should work flawlessly as it does not alter HTML structure. I limited the width on large viewports to 1000px, feel free to adjust.

[lunny]

make L-G-T-M work

[silverwind]

@Gusted can you review once more?

[Gusted]

Alright, this looks good to me as well.