Summary

Fix a couple mobile bugs on the unified banner's review menu.

Review Request #14130 — Created Aug. 29, 2024 and submitted Sept. 18, 2024, 9:33 a.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

~~14171~~

Reviewers

Groups

reviewboard

People

Description

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 the closeMenu()
method that exists on the parent Actions.MenuActionView and put the
overlay removal logic in there. However that method doesn't actually
always get called when the menu closes. The grandparent Ink MenuView
sometimes closes the menu directly through its close() method. We may
want to remove or rework these Actions.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.

Testing Done


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.

Commits

Summary	ID
Fix a couple mobile bugs on the unified banner's review menu. 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 subclasses the `closeMenu()` method that exists on the parent `Actions.MenuActionView` and put the overlay removal logic in there. However that method doesn't actually always get called when the menu closes. The grandparent Ink `MenuView` sometimes closes the menu directly through its `close()` method. We may want to remove or rework these `Actions.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 fixes this by forcing the menu contents to take up the width of the screen and wrap any text in it.	62e2ea7bdcb15660fc0043d9652c041b4f024dfc

Summary

Fix a couple mobile bugs on the unified banner's review menu.

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 subclasses the `closeMenu()` method that exists on the parent `Actions.MenuActionView` and put the overlay removal logic in there. However that method doesn't actually always get called when the menu closes. The grandparent Ink `MenuView` sometimes closes the menu directly through its `close()` method. We may want to remove or rework these `Actions.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 fixes this by forcing the menu contents to take up the width of the screen and wrap any text in it.

62e2ea7bdcb15660fc0043d9652c041b4f024dfc

Files

Issues

Description	From	Last Updated
Typo in the description: "we subclasses" -> "we subclassed"	chipx86	Sept. 13, 2024, 10:05 a.m.
Does this permanently alter the wdith and other information? If you trigger this in a mobile width, then close the …	chipx86	Sept. 13, 2024, 4:21 p.m.
We should aim to use default styles but not assume them, in case those need to change down the road. …	chipx86	Sept. 14, 2024, 12:26 p.m.

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

The issue has been resolved. Show all issues

Typo in the description: "we subclasses" -> "we subclassed"

reviewboard/static/rb/js/reviews/views/reviewRequestActions.ts (Diff revision 1)

The issue has been resolved. Show all issues

Does this permanently alter the wdith and other information? If you trigger this in a mobile width, then close the menu, resize to a desktop view, and then open the menu, what happens?

maubin Sept. 13, 2024, 4:30 p.m.

Yes it does. The new changes here and in /r/14171/ address this.

Change Summary:


Reorded function definitions so that private ones come after protected ones.
Reverts back to default styling when we switch out of mobile view.

Description:

		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 subclasses the `closeMenu()`
	~	the overlay that way. Previously, we subclassed the `closeMenu()`
		method that exists on the parent `Actions.MenuActionView` and put the
		overlay removal logic in there. However that method doesn't actually
		always get called when the menu closes. The grandparent Ink `MenuView`
		sometimes closes the menu directly through its `close()` method. We may
		want to remove or rework these `Actions.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.

Commits:

	Summary	ID
	Fix a couple mobile bugs on the unified banner's review menu. 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 subclasses the `closeMenu()` method that exists on the parent `Actions.MenuActionView` and put the overlay removal logic in there. However that method doesn't actually always get called when the menu closes. The grandparent Ink `MenuView` sometimes closes the menu directly through its `close()` method. We may want to remove or rework these `Actions.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 fixes this by forcing the menu contents to take up the width of the screen and wrap any text in it.	03085618495099e1fb0eaffb670c8fd11b58274a
	Fix a couple mobile bugs on the unified banner's review menu. 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 subclasses the `closeMenu()` method that exists on the parent `Actions.MenuActionView` and put the overlay removal logic in there. However that method doesn't actually always get called when the menu closes. The grandparent Ink `MenuView` sometimes closes the menu directly through its `close()` method. We may want to remove or rework these `Actions.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 fixes this by forcing the menu contents to take up the width of the screen and wrap any text in it.	9c8c4873be3450ff513c5534f33a107fcf3c8257

Depends On:

~~14171 - Make the menu action views responsive to changing browser widths.~~

Diff:

Revision 2 (+110 -22)

Show changes

reviewboard/static/rb/js/reviews/views/reviewRequestActions.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/reviews/views/reviewRequestActions.ts (Diff revision 2)

The issue has been dropped. Show all issues

We should aim to use default styles but not assume them, in case those need to change down the road. There's a way with .css(). I think setting them to an empty string will remove the override, unless it's null.

chipx86 Sept. 14, 2024, 12:26 p.m.

Oh I was thinking we were overriding Ink CSS. Nevermind.

maubin Sept. 16, 2024, 8:51 a.m.

Actually this is what I wanted to do, I wanted to revert back to whatever value was there before we overrode it. I didn't know you could remove the inline styling by setting the values to an empty string.

I'm going to go with what you suggested just to be safer and make sure we're actually going back to whatever was set in any css file instead of overriding it again with our own "default".

Ship it!

```
Ship It!
```

Commits:

	Summary	ID
	Fix a couple mobile bugs on the unified banner's review menu. 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 subclasses the `closeMenu()` method that exists on the parent `Actions.MenuActionView` and put the overlay removal logic in there. However that method doesn't actually always get called when the menu closes. The grandparent Ink `MenuView` sometimes closes the menu directly through its `close()` method. We may want to remove or rework these `Actions.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 fixes this by forcing the menu contents to take up the width of the screen and wrap any text in it.	9c8c4873be3450ff513c5534f33a107fcf3c8257
	Fix a couple mobile bugs on the unified banner's review menu. 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 subclasses the `closeMenu()` method that exists on the parent `Actions.MenuActionView` and put the overlay removal logic in there. However that method doesn't actually always get called when the menu closes. The grandparent Ink `MenuView` sometimes closes the menu directly through its `close()` method. We may want to remove or rework these `Actions.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 fixes this by forcing the menu contents to take up the width of the screen and wrap any text in it.	62e2ea7bdcb15660fc0043d9652c041b4f024dfc

Diff:

Revision 3 (+110 -22)

Show changes

reviewboard/static/rb/js/reviews/views/reviewRequestActions.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.x (4aa19fe)