From c47015b861fa538003d4d7d1267f9acedc6d2948 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 21 Feb 2023 09:59:56 +0800 Subject: [PATCH] DEV: Change `enable_sidebar` query param to `navigation_menu` (#20368) What does this change do? This commit the client to override the navigation menu setting configured by the site temporarily based on the value of the `navigation_menu` query param. The new query param replaces the old `enable_sidebar` query param. Why do we need this change? The motivation here is to allow theme maintainers to quickly preview what the site will look like with the various navigation menu site setting. --- .../javascripts/bootstrap-json/index.js | 6 +-- .../discourse/app/components/site-header.js | 1 + .../discourse/app/controllers/application.js | 15 +++--- .../discourse/app/templates/application.hbs | 1 + .../discourse/app/widgets/header.js | 10 ++-- .../tests/acceptance/sidebar-user-test.js | 39 ---------------- app/controllers/application_controller.rb | 2 +- .../concerns/user_sidebar_mixin.rb | 3 +- .../components/legacy_header_dropdown.rb | 15 ++++++ .../components/sidebar_header_dropdown.rb | 15 ++++++ spec/system/viewing_sidebar_spec.rb | 46 +++++++++++++++---- 11 files changed, 90 insertions(+), 63 deletions(-) create mode 100644 spec/system/page_objects/components/legacy_header_dropdown.rb create mode 100644 spec/system/page_objects/components/sidebar_header_dropdown.rb diff --git a/app/assets/javascripts/bootstrap-json/index.js b/app/assets/javascripts/bootstrap-json/index.js index b795588ce70..323a16816d3 100644 --- a/app/assets/javascripts/bootstrap-json/index.js +++ b/app/assets/javascripts/bootstrap-json/index.js @@ -252,9 +252,9 @@ async function buildFromBootstrap(proxy, baseURL, req, response, preload) { url.searchParams.append("safe_mode", reqUrlSafeMode); } - const enableSidebar = forUrlSearchParams.get("enable_sidebar"); - if (enableSidebar) { - url.searchParams.append("enable_sidebar", enableSidebar); + const navigationMenu = forUrlSearchParams.get("navigation_menu"); + if (navigationMenu) { + url.searchParams.append("navigation_menu", navigationMenu); } const reqUrlPreviewThemeId = forUrlSearchParams.get("preview_theme_id"); diff --git a/app/assets/javascripts/discourse/app/components/site-header.js b/app/assets/javascripts/discourse/app/components/site-header.js index b8c146d9325..66e85aa7660 100644 --- a/app/assets/javascripts/discourse/app/components/site-header.js +++ b/app/assets/javascripts/discourse/app/components/site-header.js @@ -343,6 +343,7 @@ const SiteHeaderComponent = MountWidget.extend( canSignUp: this.canSignUp, sidebarEnabled: this.sidebarEnabled, showSidebar: this.showSidebar, + navigationMenuQueryParamOverride: this.navigationMenuQueryParamOverride, }; }, diff --git a/app/assets/javascripts/discourse/app/controllers/application.js b/app/assets/javascripts/discourse/app/controllers/application.js index 1c775ad51bb..653b509415d 100644 --- a/app/assets/javascripts/discourse/app/controllers/application.js +++ b/app/assets/javascripts/discourse/app/controllers/application.js @@ -7,13 +7,13 @@ import { action } from "@ember/object"; const HIDE_SIDEBAR_KEY = "sidebar-hidden"; export default Controller.extend({ - queryParams: [{ sidebarQueryParamOverride: "enable_sidebar" }], + queryParams: [{ navigationMenuQueryParamOverride: "navigation_menu" }], showTop: true, showFooter: false, router: service(), showSidebar: false, - sidebarQueryParamOverride: null, + navigationMenuQueryParamOverride: null, sidebarDisabledRouteOverride: false, showSiteHeader: true, @@ -63,13 +63,13 @@ export default Controller.extend({ }, @discourseComputed( - "sidebarQueryParamOverride", + "navigationMenuQueryParamOverride", "siteSettings.navigation_menu", "canDisplaySidebar", "sidebarDisabledRouteOverride" ) sidebarEnabled( - sidebarQueryParamOverride, + navigationMenuQueryParamOverride, navigationMenu, canDisplaySidebar, sidebarDisabledRouteOverride @@ -82,11 +82,14 @@ export default Controller.extend({ return false; } - if (sidebarQueryParamOverride === "1") { + if (navigationMenuQueryParamOverride === "sidebar") { return true; } - if (sidebarQueryParamOverride === "0") { + if ( + navigationMenuQueryParamOverride === "legacy" || + navigationMenuQueryParamOverride === "header_dropdown" + ) { return false; } diff --git a/app/assets/javascripts/discourse/app/templates/application.hbs b/app/assets/javascripts/discourse/app/templates/application.hbs index f5eca4abcb3..8958f774b33 100644 --- a/app/assets/javascripts/discourse/app/templates/application.hbs +++ b/app/assets/javascripts/discourse/app/templates/application.hbs @@ -17,6 +17,7 @@ @toggleAnonymous={{route-action "toggleAnonymous"}} @logout={{route-action "logout"}} @sidebarEnabled={{this.sidebarEnabled}} + @navigationMenuQueryParamOverride={{this.navigationMenuQueryParamOverride}} @showSidebar={{this.showSidebar}} @toggleSidebar={{action "toggleSidebar"}} /> diff --git a/app/assets/javascripts/discourse/app/widgets/header.js b/app/assets/javascripts/discourse/app/widgets/header.js index 50be1ea06a7..12a47387490 100644 --- a/app/assets/javascripts/discourse/app/widgets/header.js +++ b/app/assets/javascripts/discourse/app/widgets/header.js @@ -509,10 +509,12 @@ export default createWidget("header", { }) ); } else if (state.hamburgerVisible) { - if (this.siteSettings.navigation_menu !== "legacy") { - if (!attrs.sidebarEnabled || this.site.narrowDesktopView) { - panels.push(this.attach("revamped-hamburger-menu-wrapper", {})); - } + if ( + attrs.navigationMenuQueryParamOverride === "header_dropdown" || + (this.siteSettings.navigation_menu !== "legacy" && + (!attrs.sidebarEnabled || this.site.narrowDesktopView)) + ) { + panels.push(this.attach("revamped-hamburger-menu-wrapper", {})); } else { panels.push(this.attach("hamburger-menu")); } diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-test.js index ed2b4c5db32..94d5b65f91f 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-test.js @@ -51,25 +51,6 @@ acceptance( "hides the sidebar dropdown" ); }); - - test("'enable_sidebar' query param override to enable sidebar", async function (assert) { - await visit("/?enable_sidebar=1"); - - assert.ok(exists(".sidebar-container"), "sidebar is displayed"); - - await click(".btn-sidebar-toggle"); - - assert.notOk( - exists(".sidebar-hamburger-dropdown"), - "does not display the sidebar dropdown" - ); - - assert.notOk(exists(".sidebar-container"), "sidebar is hidden"); - - await click(".btn-sidebar-toggle"); - - assert.ok(exists(".sidebar-container"), "sidebar is displayed"); - }); } ); @@ -132,26 +113,6 @@ acceptance( assert.ok(exists(".sidebar-container"), "displays the sidebar"); }); - test("'enable_sidebar' query param override to disable sidebar", async function (assert) { - await visit("/?enable_sidebar=0"); - - assert.notOk(exists(".sidebar-container"), "sidebar is not displayed"); - - await click(".hamburger-dropdown"); - - assert.ok( - exists(".sidebar-hamburger-dropdown"), - "displays the sidebar dropdown" - ); - - await click(".hamburger-dropdown"); - - assert.notOk( - exists(".sidebar-hamburger-dropdown"), - "hides the sidebar dropdown" - ); - }); - test("button to toggle between mobile and desktop view on touch devices ", async function (assert) { const capabilities = this.container.lookup("capabilities:main"); capabilities.touch = true; diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1451e7328b0..80b6a77e7c2 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -645,7 +645,7 @@ class ApplicationController < ActionController::Base current_user, scope: guardian, root: false, - enable_sidebar_param: params[:enable_sidebar], + navigation_menu_param: params[:navigation_menu], ), ), ) diff --git a/app/serializers/concerns/user_sidebar_mixin.rb b/app/serializers/concerns/user_sidebar_mixin.rb index 8f74c096e47..8266a2f1f0f 100644 --- a/app/serializers/concerns/user_sidebar_mixin.rb +++ b/app/serializers/concerns/user_sidebar_mixin.rb @@ -59,6 +59,7 @@ module UserSidebarMixin private def sidebar_navigation_menu? - !SiteSetting.legacy_navigation_menu? || options[:enable_sidebar_param] == "1" + !SiteSetting.legacy_navigation_menu? || + %w[sidebar header_dropdown].include?(options[:navigation_menu_param]) end end diff --git a/spec/system/page_objects/components/legacy_header_dropdown.rb b/spec/system/page_objects/components/legacy_header_dropdown.rb new file mode 100644 index 00000000000..f04fd68ce2f --- /dev/null +++ b/spec/system/page_objects/components/legacy_header_dropdown.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module PageObjects + module Components + class LegacyHeaderDropdown < PageObjects::Components::Base + def click + page.find(".hamburger-dropdown").click + end + + def visible? + page.has_css?(".menu-panel.drop-down") + end + end + end +end diff --git a/spec/system/page_objects/components/sidebar_header_dropdown.rb b/spec/system/page_objects/components/sidebar_header_dropdown.rb new file mode 100644 index 00000000000..fa86f1c72f3 --- /dev/null +++ b/spec/system/page_objects/components/sidebar_header_dropdown.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module PageObjects + module Components + class SidebarHeaderDropdown < PageObjects::Components::Base + def click + page.find(".hamburger-dropdown").click + end + + def visible? + page.has_css?(".revamped.menu-panel.drop-down") + end + end + end +end diff --git a/spec/system/viewing_sidebar_spec.rb b/spec/system/viewing_sidebar_spec.rb index 5dfe75f87a3..48b0faf4485 100644 --- a/spec/system/viewing_sidebar_spec.rb +++ b/spec/system/viewing_sidebar_spec.rb @@ -5,13 +5,13 @@ describe "Viewing sidebar", type: :system, js: true do fab!(:user) { Fabricate(:user) } fab!(:category_sidebar_section_link) { Fabricate(:category_sidebar_section_link, user: user) } + before_all { sign_in(user) } + describe "when using the legacy navigation menu" do before { SiteSetting.navigation_menu = "legacy" } - it "should display the sidebar when `enable_sidebar` query param is '1'" do - sign_in(user) - - visit("/latest?enable_sidebar=1") + it "should display the sidebar when `navigation_menu` query param is 'sidebar'" do + visit("/latest?navigation_menu=sidebar") sidebar = PageObjects::Components::Sidebar.new @@ -19,20 +19,48 @@ describe "Viewing sidebar", type: :system, js: true do expect(sidebar).to have_category_section_link(category_sidebar_section_link.linkable) expect(page).not_to have_css(".hamburger-dropdown") end + + it "should display the sidebar dropdown menu when `navigation_menu` query param is 'header_dropdown'" do + visit("/latest?navigation_menu=header_dropdown") + + sidebar = PageObjects::Components::Sidebar.new + + expect(sidebar).to be_not_visible + + header_dropdown = PageObjects::Components::SidebarHeaderDropdown.new + header_dropdown.click + + expect(header_dropdown).to be_visible + end end describe "when using the sidebar navigation menu" do before { SiteSetting.navigation_menu = "sidebar" } - it "should not display the sidebar when `enable_sidebar` query param is '0'" do - sign_in(user) - - visit("/latest?enable_sidebar=0") + it "should display the legacy dropdown menu when `navigation_menu` query param is 'legacy'" do + visit("/latest?navigation_menu=legacy") sidebar = PageObjects::Components::Sidebar.new expect(sidebar).to be_not_visible - expect(page).to have_css(".hamburger-dropdown") + + legacy_header_dropdown = PageObjects::Components::LegacyHeaderDropdown.new + legacy_header_dropdown.click + + expect(legacy_header_dropdown).to be_visible + end + + it "should display the sidebar dropdown menu when `navigation_menu` query param is 'header_dropdown'" do + visit("/latest?navigation_menu=header_dropdown") + + sidebar = PageObjects::Components::Sidebar.new + + expect(sidebar).to be_not_visible + + header_dropdown = PageObjects::Components::SidebarHeaderDropdown.new + header_dropdown.click + + expect(header_dropdown).to be_visible end end end