DEV: Fix edit nav menu modals not appearing on mobile (#22403)

What is the problem?

This regressed in fe294ab1a7 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.

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}}
-</Sidebar::Section>
+</Sidebar::Section>
-
-{{#if this.isModalVisible}}
-  <Sidebar::EditNavigationMenu::CategoriesModal
-    @closeModal={{fn (mut this.isModalVisible) false}}
-  />
-{{/if}}

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);
+  }
 }

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}}
-</Sidebar::Section>
+</Sidebar::Section>
-
-{{#if this.isModalVisible}}
-  <Sidebar::EditNavigationMenu::TagsModal
-    @closeModal={{fn (mut this.isModalVisible) false}}
-  />
-{{/if}}

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);
+  }
 }

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
+  shared_examples "a user can edit the sidebar categories navigation" do |mobile|
-    visit "/latest"
+    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"))
+      modal = sidebar.click_edit_categories_button
-    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
 
-    expect(modal).to have_categories(
+      expect(modal).to have_right_title(I18n.t("js.sidebar.categories_form_modal.title"))
-      [category, category_subcategory, category_subcategory2, category2, category2_subcategory],
+      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
+      expect(modal).to have_categories(
-      .toggle_category_checkbox(category)
+        [category, category_subcategory, category_subcategory2, category2, category2_subcategory],
-      .toggle_category_checkbox(category_subcategory2)
+      )
-      .toggle_category_checkbox(category2)
-      .save
 
-    expect(modal).to be_closed
+      modal
-    expect(sidebar).to have_section_link(category.name)
+        .toggle_category_checkbox(category)
-    expect(sidebar).to have_section_link(category_subcategory2.name)
+        .toggle_category_checkbox(category_subcategory2)
-    expect(sidebar).to have_section_link(category2.name)
+        .toggle_category_checkbox(category2)
+        .save
 
-    visit "/latest"
+      expect(modal).to be_closed
 
-    expect(sidebar).to have_categories_section
+      sidebar.open_on_mobile if mobile
-    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
+      expect(sidebar).to have_section_link(category.name)
-    modal.toggle_category_checkbox(category_subcategory2).toggle_category_checkbox(category2).save
+      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)
+      sidebar.open_on_mobile if mobile
-    expect(sidebar).to have_no_section_link(category_subcategory2.name)
+
-    expect(sidebar).to have_no_section_link(category2.name)
+      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

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
+  shared_examples "a user can edit the sidebar tags navigation" do |mobile|
-    visit "/latest"
+    it "allows a user to edit the sidebar tags navigation", mobile: mobile do
+      visit "/latest"
 
-    expect(sidebar).to have_tags_section
+      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_section_link(tag3.name)
-    expect(sidebar).to have_section_link(tag4.name)
 
-    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"))
+      modal = sidebar.click_edit_tags_button
-    try_until_success { expect(modal).to have_focus_on_filter_input }
-    expect(modal).to have_tag_checkboxes([tag1, tag2, tag3, tag4])
 
-    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
+      modal.toggle_tag_checkbox(tag1).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)
 
-    visit "/latest"
+      expect(modal).to be_closed
 
-    expect(sidebar).to have_section_link(tag1.name)
+      sidebar.open_on_mobile if mobile
-    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
+      expect(sidebar).to have_section_link(tag1.name)
-    modal.toggle_tag_checkbox(tag2).save
+      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)
+      sidebar.open_on_mobile if mobile
-    expect(sidebar).to have_no_section_link(tag2.name)
+
-    expect(sidebar).to have_no_section_link(tag3.name)
+      expect(sidebar).to have_section_link(tag1.name)
-    expect(sidebar).to have_no_section_link(tag4.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

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