Make the comment dialog look better on mobile.
Review Request #14188 — Created Sept. 25, 2024 and submitted
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 |
---|---|
2110e8a5fbdb024da44488370dfb04db4de3fe7f |
Description | From | Last Updated |
---|---|---|
Can you check what happens on a mobile device in landscape mode? |
david | |
Since we're alreaedy sharing the showComments in the if section, how do you feel about removing some duplication? if (showComments) … |
OfficeStapler | |
We might as well optimistically change this to use the new var(--ink-z-menu) |
david | |
And var(--ink-z-dialog) here. |
david | |
Instead of "mobile" vs "not mobile", perhaps we should call this portrait vs landscape layout? |
david | |
We might as well just call this inline, since we only use it once. |
david | |
Missing Returns: |
david | |
This needs a type annotation. |
david | |
Is there any reason not to make this #private? |
david | |
Since we're already defining height here, how do you feel about just assigning it to CommentDialogView.DIALOG_NON_EDITABLE_HEIGHT? |
OfficeStapler | |
I noticed in the screenshots that the margin around the other-comments box is too small on the bottom. Can we … |
david | |
There's an extra space before the : |
david | |
Is this taken care of automatically in draggable()? |
chipx86 |
-
-
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
- Commits:
-
Summary ID ee494e3c9616b0f73039605881a6f9bfaadf3b7f 28acbe973b9941b53e00df2280fc3eb53be3741f
Checks run (2 succeeded)
- Change Summary:
-
- Uses Ink z indices.
- Added a max height for the dialog.
- Gave the draft form a bit more height so the textbox is not as small.
- Checks the height of the screen when deciding whether to display the vertical
or horizontal version of the dialog.
- Commits:
-
Summary ID 28acbe973b9941b53e00df2280fc3eb53be3741f 5cb4bae9072b7452adc9daee4864770758215a7b - Diff:
-
Revision 3 (+118 -38)
- Added Files:
Checks run (2 succeeded)
- Commits:
-
Summary ID 5cb4bae9072b7452adc9daee4864770758215a7b acda85bc19bbded68ae45fd185535cd23b9e67b2
Checks run (2 succeeded)
- Commits:
-
Summary ID acda85bc19bbded68ae45fd185535cd23b9e67b2 f3957ac4b435c5516df5435d1622954d08b590d3
Checks run (2 succeeded)
- Commits:
-
Summary ID f3957ac4b435c5516df5435d1622954d08b590d3 83a61e53a15a726e712457f23f1401d3ffd0b063