Make the comment dialog look better on mobile.

Review Request #14188 — Created Sept. 25, 2024 and submitted

Information

Review Board
release-7.x

Reviewers

Currently, the comment dialog looks very squished when viewing it on mobile
with published comments. This change fixes this by stacking the published
comments box on top of the draft comment form, instead of side by side. We
now allow the dialog to be dragged from the "Other comments" header in
addition to the draft form header. It's more natural to try to drag the
top "Other comments" header on mobile when trying to move the dialog.

This also updates the z-index for the prev and next file attachment nav
buttons, so that it has a lower z-index than the comment dialog.

Tested the following on mobile and on desktop:
- Creating pure draft comments.
- Creating comments that are on the same region as an existing comment on
an image.
- Creating comments that are on the same region as an existing comment on
a diff.
- Creating comments that are on the same region as an existing comment on
a markdown file.
- Clicking on an existing comment while not logged in.
- Moving and resizing the comment dialog.

Summary ID
Make the comment dialog look better on mobile.
Currently, the comment dialog looks very squished when viewing it on mobile with published comments. This change fixes this by stacking the published comments box on top of the draft comment form, instead of side by side. This also updates the z-index for the prev and next file attachment nav buttons, so that it has a lower z-index than the comment dialog.
2110e8a5fbdb024da44488370dfb04db4de3fe7f

Description From Last Updated

Can you check what happens on a mobile device in landscape mode?

daviddavid

Since we're alreaedy sharing the showComments in the if section, how do you feel about removing some duplication? if (showComments) …

OfficeStaplerOfficeStapler

We might as well optimistically change this to use the new var(--ink-z-menu)

daviddavid

And var(--ink-z-dialog) here.

daviddavid

Instead of "mobile" vs "not mobile", perhaps we should call this portrait vs landscape layout?

daviddavid

We might as well just call this inline, since we only use it once.

daviddavid

Missing Returns:

daviddavid

This needs a type annotation.

daviddavid

Is there any reason not to make this #private?

daviddavid

Since we're already defining height here, how do you feel about just assigning it to CommentDialogView.DIALOG_NON_EDITABLE_HEIGHT?

OfficeStaplerOfficeStapler

I noticed in the screenshots that the margin around the other-comments box is too small on the bottom. Can we …

daviddavid

There's an extra space before the :

daviddavid

Is this taken care of automatically in draggable()?

chipx86chipx86
OfficeStapler
  1. 
      
  2. reviewboard/static/rb/js/reviews/views/commentDialogView.ts (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Since we're alreaedy sharing the showComments in the if section, how do you feel about removing some duplication?

    if (showComments) {
      let moveHeight = height;
      let moveWidth = width;
      if (inMobileMode) {
        moveHeight = CommentDialogView.COMMENTS_BOX_HEIGHT_MOBILE;
        draftFormY = ...;
        ...
      } else {
        moveWidth = CommentDialogView.COMMENTS_BOX_WIDTH;
        commentsWidth = ...;
        ...
      }
      this._$commentsPane
                    .outerWidth(moveWidth)
                    .outerHeight(moveHeight)
                    .move(0, 0, 'absolute');
    
      const $commentsList = this.commentsList.$el;
                $commentsList.height(this._$commentsPane.height() -
                                     $commentsList.position().top);
    }
    

    Not too sure if the order of the commentsPane matters though

    1. Good idea, makes things much cleaner.

  3. 
      
maubin
david
  1. 
      
  2. Show all issues

    Can you check what happens on a mobile device in landscape mode?

    1. It would display this vertical version of the dialog which wasn't too pretty. I've updated the change so that we only display the vertical version of the dialog if it fits within the screen height.

  3. Show all issues

    We might as well optimistically change this to use the new var(--ink-z-menu)

  4. Show all issues

    And var(--ink-z-dialog) here.

  5. 
      
maubin
david
  1. 
      
  2. Show all issues

    Instead of "mobile" vs "not mobile", perhaps we should call this portrait vs landscape layout?

  3. Show all issues

    We might as well just call this inline, since we only use it once.

    1. I actually use it twice, on 847 and 851.

  4. Show all issues

    Missing Returns:

  5. Show all issues

    This needs a type annotation.

  6. 
      
maubin
david
  1. 
      
  2. Show all issues

    Is there any reason not to make this #private?

    1. I don't think so, I'll change it.

  3. 
      
maubin
OfficeStapler
  1. 
      
  2. Show all issues

    Since we're already defining height here, how do you feel about just assigning it to CommentDialogView.DIALOG_NON_EDITABLE_HEIGHT?

  3. 
      
maubin
OfficeStapler
  1. Ship It!
  2. 
      
david
  1. 
      
  2. Show all issues

    I noticed in the screenshots that the margin around the other-comments box is too small on the bottom. Can we add box-sizing: border-box to this, to fix that?

    1. I tried using box-sizing: border-box and it didn't change anything. I found that it was the margin on the inner ul element that was causing the bottom margin to be too small.

  3. Show all issues

    There's an extra space before the :

  4. 
      
maubin
chipx86
  1. 
      
  2. Show all issues

    Is this taken care of automatically in draggable()?

    1. No it's not. I removed it here because I instead set it in the reviews.less file (line 1654-1657).

  3. 
      
david
  1. Ship It!
  2. 
      
maubin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-7.x (57e2899)