Fix a couple mobile bugs on the unified banner's review menu.
Review Request #14130 — Created Aug. 29, 2024 and submitted — Latest diff uploaded
We had a couple bugs when using the review menu on mobile.
The first has to do with the event overlay that we add to the page when the
review menu opens. The overlay wasn't going away when closing the menu by
tapping somewhere else on the unified banner (i.e. tapping the mode selector,
publish buttons or empty space). And, if you opened the review menu again, a
new overlay would be added, and the overlays would keep stacking if you
repeated this.This is fixed by listening to the closing event on the menu and removing
the overlay that way. Previously, we subclassed thecloseMenu()
method that exists on the parentActions.MenuActionView
and put the
overlay removal logic in there. However that method doesn't actually
always get called when the menu closes. The grandparent InkMenuView
sometimes closes the menu directly through itsclose()
method. We may
want to remove or rework theseActions.MenuActionView.openMenu()
and
closeMenu()
methods in the future, since it could lead to unexpected
state leftover like it did here.Secondly, the contents of the menu would get cut off when viewing on
mobile. This change makes things look much better by forcing the menu
contents to take up the entire width of the screen and wrap any text
in it.
- Tested opening, closing, and selecting items in the review menu in
mobile and desktop. - Tested exiting the review menu by it and a lot of other places.
- Tested scrolling with the menu open.
- Ran JS unit tests.
reviewboard/static/rb/js/reviews/views/reviewRequestActions.ts | |||
---|---|---|---|
Revision 065a7d6cec5da8eeb8cba9ee7f37dd1412d3f8fd | New Change | ||
501 lines | |||
502 | 502 | ||
503 |
/** The event overlay when the menu is shown in mobile mode. */ |
503 |
/** The event overlay when the menu is shown in mobile mode. */ |
504 |
#overlay: OverlayView = null; |
504 |
#overlay: OverlayView = null; |
505 | 505 | ||
506 |
/** |
506 |
/** |
507 |
* Close the menu.
|
507 |
* Render the view.
|
508 |
*/
|
508 |
*/
|
509 |
protected closeMenu() { |
509 |
protected onInitialRender() { |
510 |
super.closeMenu(); |
510 |
super.onInitialRender(); |
511 | 511 | ||
512 |
Moved to line 576
if (this.#overlay) { |
512 |
this.listenTo(this.menu, 'closing', this._removeOverlay); |
513 |
this.#overlay.remove(); |
||
514 |
this.#overlay = null; |
||
515 |
} |
||
516 |
} |
513 |
} |
517 | 514 | ||
518 |
/** |
515 |
/** |
519 |
* Handle a touchstart event.
|
516 |
* Handle a touchstart event.
|
520 |
*
|
517 |
*
|
521 |
* Args:
|
518 |
* Args:
|
522 |
* e (TouchEvent):
|
519 |
* e (TouchEvent):
|
523 |
* The touch event.
|
520 |
* The touch event.
|
524 |
*/
|
521 |
*/
|
525 |
protected onTouchStart(e: TouchEvent) { |
522 |
protected onTouchStart(e: TouchEvent) { |
526 |
super.onTouchStart(e); |
523 |
super.onTouchStart(e); |
527 | 524 | ||
528 |
if (this.menu.isOpen) { |
525 |
if (this.menu.isOpen) { |
526 |
if (!this.#overlay) { |
||
529 |
this.#overlay = new OverlayView(); |
527 |
>>>> this.#overlay = new OverlayView(); |
530 |
this.#overlay.$el.appendTo('body'); |
528 |
>>>> this.#overlay.$el.appendTo('body'); |
531 | 529 | ||
532 |
this.listenTo(this.#overlay, 'click', () => { |
530 |
>>>> this.listenTo(this.#overlay, 'click', () => { |
533 |
this.closeMenu(); |
531 |
>>>> this.closeMenu(); |
534 |
}); |
532 |
>>>> }); |
535 |
} |
533 |
>>>> } |
536 |
} |
534 |
>>>> } |
537 |
}
|
535 |
>>>>} |
538 | 536 | ||
537 |
/** |
||
538 |
* Position the menu.
|
||
539 |
*
|
||
540 |
* Version Added:
|
||
541 |
* 7.0.3
|
||
542 |
*/
|
||
543 |
protected positionMenu() { |
||
544 |
const $menuEl = this.menu.$el; |
||
545 | |||
546 |
if (RB.PageManager.getPage().inMobileMode) { |
||
547 |
/* |
||
548 |
* Make the review menu take up the full width of the screen
|
||
549 |
* when on mobile.
|
||
550 |
*
|
||
551 |
* This needs to happen before the call to the parent class so
|
||
552 |
* that the parent uses the updated width for the menu.
|
||
553 |
*/
|
||
554 |
$menuEl.css({ |
||
555 |
'text-wrap': 'wrap', |
||
556 |
width: $(window).width(), |
||
557 |
}); |
||
558 |
} else { |
||
559 |
/* Use default styling on desktop. */ |
||
560 |
$menuEl.css({ |
||
561 |
'text-wrap': '', |
||
562 |
width: '', |
||
563 |
}); |
||
564 |
} |
||
565 | |||
566 |
super.positionMenu(); |
||
567 |
} |
||
568 | |||
569 |
/** |
||
570 |
* Remove the event overlay that's shown in mobile mode.
|
||
571 |
*
|
||
572 |
* Version Added:
|
||
573 |
* 7.0.3
|
||
574 |
*/
|
||
575 |
private _removeOverlay() { |
||
576 |
Moved from line 512
if (this.#overlay) { |
||
577 |
this.#overlay.remove(); |
||
578 |
this.#overlay = null; |
||
579 |
} |
||
580 |
} |
||
581 |
}
|
||
582 | |||
539 | 583 | ||
540 |
/**
|
584 |
/**
|
541 |
* Action view for the "Add File" command.
|
585 |
* Action view for the "Add File" command.
|
542 |
*
|
586 |
*
|
543 |
* Version Added:
|
587 |
* Version Added:
|
213 lines |