From cec68b3e2c1e779b1fa0a70dc57bae045df4aebd Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 28 Jun 2023 09:58:47 +1000 Subject: [PATCH] FEATURE: Conditionally change back button route for thread (#22129) When clicking back from a thread, we want to either go back to the channel if the thread was opened from an indicator, or to the thread list if we opened it from there. Since ember doesn't give a nice way to get the previous route, we need to store this ourselves. We only do this on mobile, on desktop we just follow existing behaviour. Also implements a chat router history. --------- Co-authored-by: Joffrey JAFFEUX --- .../components/chat-drawer/thread.js | 4 +- .../components/chat/thread/header.hbs | 5 +- .../components/chat/thread/header.js | 20 ++++++ .../discourse/initializers/chat-setup.js | 9 +++ .../discourse/routes/chat-channel-thread.js | 1 - .../discourse/routes/chat-channel-threads.js | 4 -- .../discourse/services/chat-drawer-router.js | 22 ++---- .../discourse/services/chat-history.js | 22 ++++++ .../discourse/services/chat-state-manager.js | 2 + .../common/chat-thread-header.scss | 2 +- plugins/chat/spec/system/navigation_spec.rb | 72 +++++++++++++++++-- .../page_objects/chat/chat_side_panel.rb | 4 ++ .../system/page_objects/chat/chat_thread.rb | 8 +-- .../chat/components/thread_list.rb | 4 ++ .../system/thread_tracking/full_page_spec.rb | 2 +- 15 files changed, 144 insertions(+), 37 deletions(-) create mode 100644 plugins/chat/assets/javascripts/discourse/services/chat-history.js diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-drawer/thread.js b/plugins/chat/assets/javascripts/discourse/components/chat-drawer/thread.js index 1a230dee760..8dd9f43f785 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-drawer/thread.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-drawer/thread.js @@ -7,14 +7,14 @@ export default class ChatDrawerThread extends Component { @service chat; @service chatStateManager; @service chatChannelsManager; - @service chatDrawerRouter; + @service chatHistory; get backLink() { const link = { models: this.chat.activeChannel.routeModels, }; - if (this.chatDrawerRouter.previousRoute?.name === "chat.channel.threads") { + if (this.chatHistory.previousRoute?.name === "chat.channel.threads") { link.title = "chat.return_to_threads_list"; link.route = "chat.channel.threads"; } else { diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/thread/header.hbs b/plugins/chat/assets/javascripts/discourse/components/chat/thread/header.hbs index 90571cc7e1e..a4fd62890af 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/thread/header.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat/thread/header.hbs @@ -2,8 +2,9 @@
{{#if @thread}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/thread/header.js b/plugins/chat/assets/javascripts/discourse/components/chat/thread/header.js index 49fac1e03e1..e47df4a5859 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/thread/header.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat/thread/header.js @@ -11,9 +11,29 @@ export default class ChatThreadHeader extends Component { @service currentUser; @service chatApi; @service router; + @service chatStateManager; + @service chatHistory; + @service site; @tracked persistedNotificationLevel = true; + get backLink() { + if ( + this.chatHistory.previousRoute?.name === "chat.channel.index" && + this.site.mobileView + ) { + return { + route: "chat.channel.index", + models: this.args.channel.routeModels, + }; + } else { + return { + route: "chat.channel.threads", + models: [], + }; + } + } + get label() { return this.args.thread.escapedTitle; } diff --git a/plugins/chat/assets/javascripts/discourse/initializers/chat-setup.js b/plugins/chat/assets/javascripts/discourse/initializers/chat-setup.js index c6dc5fbfeca..88debde523f 100644 --- a/plugins/chat/assets/javascripts/discourse/initializers/chat-setup.js +++ b/plugins/chat/assets/javascripts/discourse/initializers/chat-setup.js @@ -18,7 +18,9 @@ export default { before: "hashtag-css-generator", initialize(container) { + this.router = container.lookup("service:router"); this.chatService = container.lookup("service:chat"); + this.chatHistory = container.lookup("service:chat-history"); this.site = container.lookup("service:site"); this.siteSettings = container.lookup("service:site-settings"); this.currentUser = container.lookup("service:current-user"); @@ -30,6 +32,13 @@ export default { } withPluginApi("0.12.1", (api) => { + api.onPageChange((path) => { + const route = this.router.recognize(path); + if (route.name.startsWith("chat.")) { + this.chatHistory.visit(route); + } + }); + api.registerHashtagType("channel", new ChannelHashtagType(container)); api.registerChatComposerButton({ diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-channel-thread.js b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-thread.js index 921304b56ae..df81128aeed 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-channel-thread.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-thread.js @@ -24,7 +24,6 @@ export default class ChatChannelThread extends DiscourseRoute { afterModel(model) { this.chat.activeChannel.activeThread = model; - this.chatThreadPane.open(model); } @action diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-channel-threads.js b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-threads.js index b813c4b865d..7599442cdba 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-channel-threads.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-threads.js @@ -25,8 +25,4 @@ export default class ChatChannelThreads extends DiscourseRoute { this.chatStateManager.closeSidePanel(); } } - - activate() { - this.chatThreadListPane.open(); - } } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-drawer-router.js b/plugins/chat/assets/javascripts/discourse/services/chat-drawer-router.js index 927c03210fe..e8da3782f22 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-drawer-router.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-drawer-router.js @@ -49,30 +49,16 @@ const ROUTES = { export default class ChatDrawerRouter extends Service { @service router; + @service chatHistory; + @tracked component = null; @tracked drawerRoute = null; @tracked params = null; - @tracked history = []; - - get previousRoute() { - if (this.history.length > 1) { - return this.history[this.history.length - 2]; - } - } - - get currentRoute() { - if (this.history.length > 0) { - return this.history[this.history.length - 1]; - } - } stateFor(route) { - this.drawerRoute?.deactivate?.(this.currentRoute); + this.drawerRoute?.deactivate?.(this.chatHistory.currentRoute); - this.history.push(route); - if (this.history.length > 10) { - this.history.shift(); - } + this.chatHistory.visit(route); this.drawerRoute = ROUTES[route.name]; this.params = this.drawerRoute?.extractParams?.(route) || route.params; diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-history.js b/plugins/chat/assets/javascripts/discourse/services/chat-history.js new file mode 100644 index 00000000000..466529d6cf7 --- /dev/null +++ b/plugins/chat/assets/javascripts/discourse/services/chat-history.js @@ -0,0 +1,22 @@ +import Service from "@ember/service"; +import { tracked } from "@glimmer/tracking"; + +export default class ChatHistory extends Service { + @tracked history; + + get previousRoute() { + if (this.history?.length > 1) { + return this.history[this.history.length - 2]; + } + } + + get currentRoute() { + if (this.history?.length > 0) { + return this.history[this.history.length - 1]; + } + } + + visit(route) { + this.history = (this.history || []).slice(-9).concat([route]); + } +} diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-state-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-state-manager.js index 853bf7bf940..88608e1a41d 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-state-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-state-manager.js @@ -21,11 +21,13 @@ export function resetChatDrawerStateCallbacks() { } export default class ChatStateManager extends Service { @service chat; + @service chatHistory; @service router; @tracked isSidePanelExpanded = false; @tracked isDrawerExpanded = false; @tracked isDrawerActive = false; + @tracked _chatURL = null; @tracked _appURL = null; diff --git a/plugins/chat/assets/stylesheets/common/chat-thread-header.scss b/plugins/chat/assets/stylesheets/common/chat-thread-header.scss index be7f7698db4..74547688cfd 100644 --- a/plugins/chat/assets/stylesheets/common/chat-thread-header.scss +++ b/plugins/chat/assets/stylesheets/common/chat-thread-header.scss @@ -8,7 +8,7 @@ align-items: center; padding-inline: 0.5rem; - .chat-thread__back-to-list { + .chat-thread__back-to-previous-route { padding: 0.5rem 0; margin-right: 0.5rem; } diff --git a/plugins/chat/spec/system/navigation_spec.rb b/plugins/chat/spec/system/navigation_spec.rb index c93d48b5914..943ced3dcbe 100644 --- a/plugins/chat/spec/system/navigation_spec.rb +++ b/plugins/chat/spec/system/navigation_spec.rb @@ -4,18 +4,22 @@ RSpec.describe "Navigation", type: :system do fab!(:category) { Fabricate(:category) } fab!(:topic) { Fabricate(:topic) } fab!(:post) { Fabricate(:post, topic: topic) } - fab!(:user) { Fabricate(:admin) } + fab!(:current_user) { Fabricate(:admin) } fab!(:category_channel) { Fabricate(:category_channel) } fab!(:category_channel_2) { Fabricate(:category_channel) } fab!(:message) { Fabricate(:chat_message, chat_channel: category_channel) } let(:chat_page) { PageObjects::Pages::Chat.new } + let(:thread_page) { PageObjects::Pages::ChatThread.new } + let(:thread_list_page) { PageObjects::Components::Chat::ThreadList.new } + let(:channel_page) { PageObjects::Pages::ChatChannel.new } + let(:side_panel_page) { PageObjects::Pages::ChatSidePanel.new } let(:sidebar_page) { PageObjects::Pages::Sidebar.new } let(:sidebar_component) { PageObjects::Components::Sidebar.new } let(:chat_drawer_page) { PageObjects::Pages::ChatDrawer.new } before do - chat_system_bootstrap(user, [category_channel, category_channel_2]) - sign_in(user) + chat_system_bootstrap(current_user, [category_channel, category_channel_2]) + sign_in(current_user) end context "when clicking chat icon and drawer is viewing channel" do @@ -127,6 +131,66 @@ RSpec.describe "Navigation", type: :system do end end + context "when opening a thread" do + fab!(:thread) { Fabricate(:chat_thread, channel: category_channel) } + + before do + SiteSetting.enable_experimental_chat_threaded_discussions = true + category_channel.update!(threading_enabled: true) + Fabricate(:chat_message, thread: thread) + thread.add(current_user) + end + + context "when opening a thread from the thread list" do + it "goes back to the thread list when clicking the back button" do + visit("/chat") + chat_page.visit_channel(category_channel) + channel_page.open_thread_list + expect(thread_list_page).to have_loaded + thread_list_page.open_thread(thread) + expect(side_panel_page).to have_open_thread(thread) + thread_page.back_to_previous_route + expect(thread_list_page).to have_loaded + end + + context "for mobile" do + it "goes back to the thread list when clicking the back button", mobile: true do + visit("/chat") + chat_page.visit_channel(category_channel) + channel_page.open_thread_list + expect(thread_list_page).to have_loaded + thread_list_page.open_thread(thread) + expect(side_panel_page).to have_open_thread(thread) + thread_page.back_to_previous_route + expect(thread_list_page).to have_loaded + end + end + end + + context "when opening a thread from indicator" do + it "goes back to the thread list when clicking the back button" do + visit("/chat") + chat_page.visit_channel(category_channel) + channel_page.message_thread_indicator(thread.original_message).click + expect(side_panel_page).to have_open_thread(thread) + thread_page.back_to_previous_route + expect(thread_list_page).to have_loaded + end + + context "for mobile" do + it "closes the thread and goes back to the channel when clicking the back button", + mobile: true do + visit("/chat") + chat_page.visit_channel(category_channel) + channel_page.message_thread_indicator(thread.original_message).click + expect(side_panel_page).to have_open_thread(thread) + thread_page.back_to_previous_route + expect(side_panel_page).not_to be_open + end + end + end + end + context "when sidebar is configured as the navigation menu" do before { SiteSetting.navigation_menu = "sidebar" } @@ -244,7 +308,7 @@ RSpec.describe "Navigation", type: :system do context "when opening a channel in full page" do fab!(:other_user) { Fabricate(:user) } - fab!(:dm_channel) { Fabricate(:direct_message_channel, users: [user, other_user]) } + fab!(:dm_channel) { Fabricate(:direct_message_channel, users: [current_user, other_user]) } it "activates the channel in the sidebar" do visit("/chat/c/#{category_channel.slug}/#{category_channel.id}") diff --git a/plugins/chat/spec/system/page_objects/chat/chat_side_panel.rb b/plugins/chat/spec/system/page_objects/chat/chat_side_panel.rb index 70b6cc8cb52..405ca6b84be 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat_side_panel.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat_side_panel.rb @@ -3,6 +3,10 @@ module PageObjects module Pages class ChatSidePanel < PageObjects::Pages::Base + def open? + has_css?(".chat-side-panel") + end + def has_open_thread?(thread = nil) if thread has_css?(".chat-side-panel .chat-thread[data-id='#{thread.id}']") diff --git a/plugins/chat/spec/system/page_objects/chat/chat_thread.rb b/plugins/chat/spec/system/page_objects/chat/chat_thread.rb index fcb023823b0..b13ac8d5e6d 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat_thread.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat_thread.rb @@ -58,17 +58,17 @@ module PageObjects header.find(".chat-thread__close").click end - def back_to_list - header.find(".chat-thread__back-to-list").click + def back_to_previous_route + header.find(".chat-thread__back-to-previous-route").click end def has_no_unread_list_indicator? - has_no_css?(".chat-thread__back-to-list .chat-thread-header-unread-indicator") + has_no_css?(".chat-thread__back-to-previous-route .chat-thread-header-unread-indicator") end def has_unread_list_indicator?(count:) has_css?( - ".chat-thread__back-to-list .chat-thread-header-unread-indicator .chat-thread-header-unread-indicator__number", + ".chat-thread__back-to-previous-route .chat-thread-header-unread-indicator .chat-thread-header-unread-indicator__number", text: count.to_s, ) end diff --git a/plugins/chat/spec/system/page_objects/chat/components/thread_list.rb b/plugins/chat/spec/system/page_objects/chat/components/thread_list.rb index 8456872a44b..706a543e1d5 100644 --- a/plugins/chat/spec/system/page_objects/chat/components/thread_list.rb +++ b/plugins/chat/spec/system/page_objects/chat/components/thread_list.rb @@ -15,6 +15,10 @@ module PageObjects component.has_no_css?(".spinner") end + def open_thread(thread) + item_by_id(thread.id).click + end + def has_thread?(thread) item_by_id(thread.id) end diff --git a/plugins/chat/spec/system/thread_tracking/full_page_spec.rb b/plugins/chat/spec/system/thread_tracking/full_page_spec.rb index 43cc15d5762..58ffbb89350 100644 --- a/plugins/chat/spec/system/thread_tracking/full_page_spec.rb +++ b/plugins/chat/spec/system/thread_tracking/full_page_spec.rb @@ -40,7 +40,7 @@ describe "Thread tracking state | full page", type: :system do channel_page.open_thread_list thread_list_page.item_by_id(thread.id).click expect(thread_page).to have_no_unread_list_indicator - thread_page.back_to_list + thread_page.back_to_previous_route expect(thread_list_page).to have_no_unread_item(thread.id) end