DEV: Ensure experimental user menu is always closed after clicking on items (#18231)

Because Discourse is a single-page application, clicks on the majority of `<a>` elements in the app need to be intercepted by JavaScript to prevent browsers' default action (full page reload). Links in the user menu - which include notifications, reviewables, bookmarks etc. - are no exception to this rule and currently clicks on these items are handled by the global [click-interceptor](1fa21ed415/app/assets/javascripts/discourse/app/lib/intercept-click.js (L20)) which calls the `preventDefault` function on the click event object and uses the `DiscourseURL.routeTo` function to route the user to the page they request.

However, for links in the user menu, there's an extra step which is to let the header know that it should close the user menu after clicking an item in the menu, but the global interceptor doesn't know that because the step is specific to links in the user menu. This can cause a bug on mobile devices where the menu remains open after clicking on a notification which results in the user having to close the menu to see the page that the notification takes them to.

This commit adds a click handler to user menu items that ensures the menu is closed when an item is clicked and navigates the user to wherever the item links to. There's a small downside to this change which is that user menu items now have their own click interceptor instead of relying on the global interceptor, i.e. duplicated logic, but since it's only a couple of lines, I think we can live with it for a while.

I did try to make the click handler of the user menu items only close the menu (call the `closeUserMenu` function), but for some reasons it caused a full page reload to happen when clicking a notification item due to some weird interactions between the header widget and the user menu. I didn't debug this thoroughly because we have plans to change the header implementation from widgets/virtual-dom to Glimmer component, which will likely resolve that weird full page reload issue and we'll be able to make the click handler just close the menu and let the global interceptor prevents the default action and do the routing.

Internal topic: t/71911/118.
This commit is contained in:
Osama Sayegh 2022-09-13 15:44:45 +03:00 committed by GitHub
parent 008b958b28
commit 8c2c96af25
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 50 additions and 4 deletions

View File

@ -5,7 +5,7 @@
{{else if this.items.length}} {{else if this.items.length}}
<ul> <ul>
{{#each this.items as |item|}} {{#each this.items as |item|}}
<UserMenu::MenuItem @item={{item}}/> <UserMenu::MenuItem @item={{item}} @closeUserMenu={{@closeUserMenu}}/>
{{/each}} {{/each}}
</ul> </ul>
<div class="panel-body-bottom"> <div class="panel-body-bottom">

View File

@ -54,7 +54,10 @@ export default class UserMenuItem extends Component {
} }
@action @action
onClick() { onClick(event) {
return this.#item.onClick(); return this.#item.onClick({
event,
closeUserMenu: this.args.closeUserMenu,
});
} }
} }

View File

@ -1,3 +1,5 @@
import DiscourseURL from "discourse/lib/url";
export default class UserMenuBaseItem { export default class UserMenuBaseItem {
get className() {} get className() {}
@ -26,4 +28,13 @@ export default class UserMenuBaseItem {
get descriptionClass() {} get descriptionClass() {}
get topicId() {} get topicId() {}
onClick({ event, closeUserMenu }) {
closeUserMenu();
const href = this.linkHref;
if (href) {
DiscourseURL.routeTo(href);
}
event.preventDefault();
}
} }

View File

@ -82,5 +82,6 @@ export default class UserMenuNotificationItem extends UserMenuBaseItem {
setTransientHeader("Discourse-Clear-Notifications", this.notification.id); setTransientHeader("Discourse-Clear-Notifications", this.notification.id);
cookie("cn", this.notification.id, { path: getURL("/") }); cookie("cn", this.notification.id, { path: getURL("/") });
} }
super.onClick(...arguments);
} }
} }

View File

@ -1,4 +1,4 @@
import { click, visit } from "@ember/test-helpers"; import { click, currentURL, visit } from "@ember/test-helpers";
import { import {
acceptance, acceptance,
exists, exists,
@ -83,6 +83,37 @@ acceptance("User menu", function (needs) {
); );
}); });
test("clicking on user menu items", async function (assert) {
await visit("/");
await click(".d-header-icons .current-user");
await click("#user-menu-button-review-queue");
await click("#quick-access-review-queue li.reviewable.pending a");
assert.strictEqual(
currentURL(),
"/review/17",
"clicking on an item results in navigation to the item's page"
);
assert.notOk(
exists(".user-menu"),
"clicking on an item closes the menu after navigating"
);
await click(".d-header-icons .current-user");
await click("#user-menu-button-review-queue");
await click("#quick-access-review-queue li.reviewable.pending a");
assert.strictEqual(
currentURL(),
"/review/17",
"clicking on the same item again keeps on the same page"
);
assert.notOk(
exists(".user-menu"),
"clicking on the same item again closes the menu"
);
});
test("tabs added via the plugin API", async function (assert) { test("tabs added via the plugin API", async function (assert) {
withPluginApi("0.1", (api) => { withPluginApi("0.1", (api) => {
api.registerUserMenuTab((UserMenuTab) => { api.registerUserMenuTab((UserMenuTab) => {