• 
      

    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?

    david david

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

    OfficeStapler OfficeStapler

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

    david david

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

    david david

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

    david david

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

    david david

    Missing Returns:

    david david

    This needs a type annotation.

    david david

    Is there any reason not to make this #private?

    david david

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

    OfficeStapler OfficeStapler

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

    david david

    There's an extra space before the :

    david david

    Is this taken care of automatically in draggable()?

    chipx86 chipx86
    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)