From 6ae4d6cd4ca9d95a017bcdb1163da0438f6375ac Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 4 Jul 2023 11:11:47 +0800 Subject: [PATCH] DEV: Fix edit nav menu modals not appearing on mobile (#22403) What is the problem? This regressed in fe294ab1a77c1026b6996550eb8c36b6f07e1b73 and we did not have any tests on mobile to catch the regression. The problem was that we were conditionally rendering the edit nav menu modals component in the sidebar. However, the sidebar is collapsed on mobile when a button is clicked. When the sidebar collapses, the edit nav menu modals ended up being destroyed with it. --- .../sidebar/user/categories-section.hbs | 10 +-- .../sidebar/user/categories-section.js | 11 ++- .../components/sidebar/user/tags-section.hbs | 10 +-- .../components/sidebar/user/tags-section.js | 11 ++- ...ting_sidebar_categories_navigation_spec.rb | 87 +++++++++++-------- .../editing_sidebar_tags_navigation_spec.rb | 77 +++++++++------- .../system/page_objects/components/sidebar.rb | 16 ++-- 7 files changed, 130 insertions(+), 92 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/sidebar/user/categories-section.hbs b/app/assets/javascripts/discourse/app/components/sidebar/user/categories-section.hbs index aa791ba1e5f..3c073d92240 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/user/categories-section.hbs +++ b/app/assets/javascripts/discourse/app/components/sidebar/user/categories-section.hbs @@ -3,7 +3,7 @@ @headerLinkText={{i18n "sidebar.sections.categories.header_link_text"}} @headerActions={{array (hash - action=(fn (mut this.isModalVisible) true) + action=this.showModal title=(i18n "sidebar.sections.categories.header_action_title") ) }} @@ -46,10 +46,4 @@ @query={{hash filter="default_navigation_menu_categories"}} /> {{/if}} - - -{{#if this.isModalVisible}} - -{{/if}} \ No newline at end of file + \ No newline at end of file diff --git a/app/assets/javascripts/discourse/app/components/sidebar/user/categories-section.js b/app/assets/javascripts/discourse/app/components/sidebar/user/categories-section.js index aec4adb2d1e..f96b0bcb74b 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/user/categories-section.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/user/categories-section.js @@ -1,10 +1,12 @@ +import { action } from "@ember/object"; import { inject as service } from "@ember/service"; -import { cached, tracked } from "@glimmer/tracking"; +import { cached } from "@glimmer/tracking"; import { debounce } from "discourse-common/utils/decorators"; import Category from "discourse/models/category"; import SidebarCommonCategoriesSection from "discourse/components/sidebar/common/categories-section"; import { hasDefaultSidebarCategories } from "discourse/lib/sidebar/helpers"; +import SidebarEditNavigationMenuCategoriesModal from "discourse/components/sidebar/edit-navigation-menu/categories-modal"; export const REFRESH_COUNTS_APP_EVENT_NAME = "sidebar:refresh-categories-section-counts"; @@ -15,8 +17,6 @@ export default class SidebarUserCategoriesSection extends SidebarCommonCategorie @service modal; @service router; - @tracked isModalVisible = false; - constructor() { super(...arguments); @@ -64,4 +64,9 @@ export default class SidebarUserCategoriesSection extends SidebarCommonCategorie get hasDefaultSidebarCategories() { return hasDefaultSidebarCategories(this.siteSettings); } + + @action + showModal() { + this.modal.show(SidebarEditNavigationMenuCategoriesModal); + } } diff --git a/app/assets/javascripts/discourse/app/components/sidebar/user/tags-section.hbs b/app/assets/javascripts/discourse/app/components/sidebar/user/tags-section.hbs index 86c35ce72a5..863a54ef2b8 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/user/tags-section.hbs +++ b/app/assets/javascripts/discourse/app/components/sidebar/user/tags-section.hbs @@ -3,7 +3,7 @@ @headerLinkText={{i18n "sidebar.sections.tags.header_link_text"}} @headerActions={{array (hash - action=(fn (mut this.isModalVisible) true) + action=this.showModal title=(i18n "sidebar.sections.tags.header_action_title") ) }} @@ -44,10 +44,4 @@ @query={{hash filter="default_navigation_menu_tags"}} /> {{/if}} - - -{{#if this.isModalVisible}} - -{{/if}} \ No newline at end of file + \ No newline at end of file diff --git a/app/assets/javascripts/discourse/app/components/sidebar/user/tags-section.js b/app/assets/javascripts/discourse/app/components/sidebar/user/tags-section.js index 256a0bdd5c6..f6cedc53594 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/user/tags-section.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/user/tags-section.js @@ -1,10 +1,12 @@ -import { cached, tracked } from "@glimmer/tracking"; +import { action } from "@ember/object"; +import { cached } from "@glimmer/tracking"; import { inject as service } from "@ember/service"; import SidebarCommonTagsSection from "discourse/components/sidebar/common/tags-section"; import TagSectionLink from "discourse/lib/sidebar/user/tags-section/tag-section-link"; import PMTagSectionLink from "discourse/lib/sidebar/user/tags-section/pm-tag-section-link"; import { hasDefaultSidebarTags } from "discourse/lib/sidebar/helpers"; +import SidebarEditNavigationMenuTagsModal from "discourse/components/sidebar/edit-navigation-menu/tags-modal"; export default class SidebarUserTagsSection extends SidebarCommonTagsSection { @service currentUser; @@ -14,8 +16,6 @@ export default class SidebarUserTagsSection extends SidebarCommonTagsSection { @service siteSettings; @service topicTrackingState; - @tracked isModalVisible = false; - constructor() { super(...arguments); @@ -78,4 +78,9 @@ export default class SidebarUserTagsSection extends SidebarCommonTagsSection { get hasDefaultSidebarTags() { return hasDefaultSidebarTags(this.siteSettings); } + + @action + showModal() { + this.modal.show(SidebarEditNavigationMenuTagsModal); + } } diff --git a/spec/system/editing_sidebar_categories_navigation_spec.rb b/spec/system/editing_sidebar_categories_navigation_spec.rb index 48ecdda446c..ba3f5234cca 100644 --- a/spec/system/editing_sidebar_categories_navigation_spec.rb +++ b/spec/system/editing_sidebar_categories_navigation_spec.rb @@ -23,51 +23,70 @@ RSpec.describe "Editing sidebar categories navigation", type: :system do before { sign_in(user) } - it "allows a user to edit the sidebar categories navigation" do - visit "/latest" + shared_examples "a user can edit the sidebar categories navigation" do |mobile| + it "allows a user to edit the sidebar categories navigation", mobile: mobile do + visit "/latest" - expect(sidebar).to have_categories_section + sidebar.open_on_mobile if mobile - modal = sidebar.click_edit_categories_button + expect(sidebar).to have_categories_section - expect(modal).to have_right_title(I18n.t("js.sidebar.categories_form_modal.title")) - try_until_success { expect(modal).to have_focus_on_filter_input } - expect(modal).to have_parent_category_color(category) - expect(modal).to have_category_description_excerpt(category) - expect(modal).to have_parent_category_color(category2) - expect(modal).to have_category_description_excerpt(category2) - expect(modal).to have_no_reset_to_defaults_button + modal = sidebar.click_edit_categories_button - expect(modal).to have_categories( - [category, category_subcategory, category_subcategory2, category2, category2_subcategory], - ) + expect(modal).to have_right_title(I18n.t("js.sidebar.categories_form_modal.title")) + try_until_success { expect(modal).to have_focus_on_filter_input } + expect(modal).to have_parent_category_color(category) + expect(modal).to have_category_description_excerpt(category) + expect(modal).to have_parent_category_color(category2) + expect(modal).to have_category_description_excerpt(category2) + expect(modal).to have_no_reset_to_defaults_button - modal - .toggle_category_checkbox(category) - .toggle_category_checkbox(category_subcategory2) - .toggle_category_checkbox(category2) - .save + expect(modal).to have_categories( + [category, category_subcategory, category_subcategory2, category2, category2_subcategory], + ) - expect(modal).to be_closed - expect(sidebar).to have_section_link(category.name) - expect(sidebar).to have_section_link(category_subcategory2.name) - expect(sidebar).to have_section_link(category2.name) + modal + .toggle_category_checkbox(category) + .toggle_category_checkbox(category_subcategory2) + .toggle_category_checkbox(category2) + .save - visit "/latest" + expect(modal).to be_closed - expect(sidebar).to have_categories_section - expect(sidebar).to have_section_link(category.name) - expect(sidebar).to have_section_link(category_subcategory2.name) - expect(sidebar).to have_section_link(category2.name) + sidebar.open_on_mobile if mobile - modal = sidebar.click_edit_categories_button - modal.toggle_category_checkbox(category_subcategory2).toggle_category_checkbox(category2).save + expect(sidebar).to have_section_link(category.name) + expect(sidebar).to have_section_link(category_subcategory2.name) + expect(sidebar).to have_section_link(category2.name) - expect(modal).to be_closed + visit "/latest" - expect(sidebar).to have_section_link(category.name) - expect(sidebar).to have_no_section_link(category_subcategory2.name) - expect(sidebar).to have_no_section_link(category2.name) + sidebar.open_on_mobile if mobile + + expect(sidebar).to have_categories_section + expect(sidebar).to have_section_link(category.name) + expect(sidebar).to have_section_link(category_subcategory2.name) + expect(sidebar).to have_section_link(category2.name) + + modal = sidebar.click_edit_categories_button + modal.toggle_category_checkbox(category_subcategory2).toggle_category_checkbox(category2).save + + expect(modal).to be_closed + + sidebar.open_on_mobile if mobile + + expect(sidebar).to have_section_link(category.name) + expect(sidebar).to have_no_section_link(category_subcategory2.name) + expect(sidebar).to have_no_section_link(category2.name) + end + end + + describe "when on desktop" do + include_examples "a user can edit the sidebar categories navigation", false + end + + describe "when on mobile" do + include_examples "a user can edit the sidebar categories navigation", true end it "displays the categories in the modal based on the fixed position of the category when `fixed_category_positions` site setting is enabled" do diff --git a/spec/system/editing_sidebar_tags_navigation_spec.rb b/spec/system/editing_sidebar_tags_navigation_spec.rb index c60e7d6c387..973719055ad 100644 --- a/spec/system/editing_sidebar_tags_navigation_spec.rb +++ b/spec/system/editing_sidebar_tags_navigation_spec.rb @@ -28,45 +28,64 @@ RSpec.describe "Editing sidebar tags navigation", type: :system do before { sign_in(user) } - it "allows a user to edit the sidebar tags navigation" do - visit "/latest" + shared_examples "a user can edit the sidebar tags navigation" do |mobile| + it "allows a user to edit the sidebar tags navigation", mobile: mobile do + visit "/latest" - expect(sidebar).to have_tags_section - expect(sidebar).to have_section_link(tag1.name) - expect(sidebar).to have_section_link(tag2.name) - expect(sidebar).to have_section_link(tag3.name) - expect(sidebar).to have_section_link(tag4.name) + sidebar.open_on_mobile if mobile - modal = sidebar.click_edit_tags_button + expect(sidebar).to have_tags_section + expect(sidebar).to have_section_link(tag1.name) + expect(sidebar).to have_section_link(tag2.name) + expect(sidebar).to have_section_link(tag3.name) + expect(sidebar).to have_section_link(tag4.name) - expect(modal).to have_right_title(I18n.t("js.sidebar.tags_form_modal.title")) - try_until_success { expect(modal).to have_focus_on_filter_input } - expect(modal).to have_tag_checkboxes([tag1, tag2, tag3, tag4]) + modal = sidebar.click_edit_tags_button - modal.toggle_tag_checkbox(tag1).toggle_tag_checkbox(tag2).save + expect(modal).to have_right_title(I18n.t("js.sidebar.tags_form_modal.title")) + try_until_success { expect(modal).to have_focus_on_filter_input } + expect(modal).to have_tag_checkboxes([tag1, tag2, tag3, tag4]) - expect(modal).to be_closed - expect(sidebar).to have_section_link(tag1.name) - expect(sidebar).to have_section_link(tag2.name) - expect(sidebar).to have_no_section_link(tag3.name) - expect(sidebar).to have_no_section_link(tag4.name) + modal.toggle_tag_checkbox(tag1).toggle_tag_checkbox(tag2).save - visit "/latest" + expect(modal).to be_closed - expect(sidebar).to have_section_link(tag1.name) - expect(sidebar).to have_section_link(tag2.name) - expect(sidebar).to have_no_section_link(tag3.name) - expect(sidebar).to have_no_section_link(tag4.name) + sidebar.open_on_mobile if mobile - modal = sidebar.click_edit_tags_button - modal.toggle_tag_checkbox(tag2).save + expect(sidebar).to have_section_link(tag1.name) + expect(sidebar).to have_section_link(tag2.name) + expect(sidebar).to have_no_section_link(tag3.name) + expect(sidebar).to have_no_section_link(tag4.name) - expect(modal).to be_closed + visit "/latest" - expect(sidebar).to have_section_link(tag1.name) - expect(sidebar).to have_no_section_link(tag2.name) - expect(sidebar).to have_no_section_link(tag3.name) - expect(sidebar).to have_no_section_link(tag4.name) + sidebar.open_on_mobile if mobile + + expect(sidebar).to have_section_link(tag1.name) + expect(sidebar).to have_section_link(tag2.name) + expect(sidebar).to have_no_section_link(tag3.name) + expect(sidebar).to have_no_section_link(tag4.name) + + modal = sidebar.click_edit_tags_button + modal.toggle_tag_checkbox(tag2).save + + expect(modal).to be_closed + + sidebar.open_on_mobile if mobile + + expect(sidebar).to have_section_link(tag1.name) + expect(sidebar).to have_no_section_link(tag2.name) + expect(sidebar).to have_no_section_link(tag3.name) + expect(sidebar).to have_no_section_link(tag4.name) + end + end + + describe "when on desktop" do + include_examples "a user can edit the sidebar tags navigation", false + end + + describe "when on mobile" do + include_examples "a user can edit the sidebar tags navigation", true end it "displays the all tags in the modal when `tags_listed_by_group` site setting is true" do diff --git a/spec/system/page_objects/components/sidebar.rb b/spec/system/page_objects/components/sidebar.rb index ccd68a80f74..5307389bf48 100644 --- a/spec/system/page_objects/components/sidebar.rb +++ b/spec/system/page_objects/components/sidebar.rb @@ -3,6 +3,10 @@ module PageObjects module Components class Sidebar < PageObjects::Components::Base + def open_on_mobile + click_button("toggle-hamburger-menu") + end + def visible? page.has_css?("#d-sidebar") end @@ -69,10 +73,12 @@ module PageObjects find("#discourse-modal-title") end - SIDEBAR_WRAPPER_SELECTOR = ".sidebar-wrapper" - def has_section?(name) - find(SIDEBAR_WRAPPER_SELECTOR).has_button?(name) + has_css?(".sidebar-sections [data-section-name='#{name.parameterize}']") + end + + def has_no_section?(name) + has_no_css?(".sidebar-sections [data-section-name='#{name.parameterize}']") end def has_categories_section? @@ -103,10 +109,6 @@ module PageObjects expect(tag_section_links.map(&:text)).to eq(tag_names) end - def has_no_section?(name) - find(SIDEBAR_WRAPPER_SELECTOR).has_no_button?(name) - end - def primary_section_links(slug) all("[data-section-name='#{slug}'] .sidebar-section-link-wrapper").map(&:text) end