From db74e9484b0432b44d02ad279a5bf7a43b757923 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Tue, 21 Mar 2023 12:23:28 +1100 Subject: [PATCH] FEATURE: ability to reorder links in custom sidebar sections (#20626) Drag and drop to reorder custom sidebar sections --- .../app/components/sidebar/section.hbs | 9 ++- .../sidebar/user/custom-sections.hbs | 13 +++ .../sidebar/user/custom-sections.js | 34 ++------ .../components/sidebar/user/section-link.js | 63 +++++++++++++++ .../app/components/sidebar/user/section.js | 79 +++++++++++++++++++ .../app/controllers/sidebar-section-form.js | 5 +- .../discourse/app/modifiers/draggable.js | 2 +- .../stylesheets/common/base/_index.scss | 1 + .../common/base/sidebar-custom-section.scss | 34 ++++++++ .../stylesheets/common/base/sidebar.scss | 13 --- .../sidebar_sections_controller.rb | 24 ++++++ app/models/sidebar_section.rb | 2 +- app/models/sidebar_section_link.rb | 8 ++ config/routes.rb | 1 + ...0_add_position_to_sidebar_section_links.rb | 12 +++ .../sidebar_sections_controller_spec.rb | 62 ++++++++++++++- spec/system/custom_sidebar_sections_spec.rb | 27 +++++++ 17 files changed, 343 insertions(+), 46 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/components/sidebar/user/section-link.js create mode 100644 app/assets/javascripts/discourse/app/components/sidebar/user/section.js create mode 100644 app/assets/stylesheets/common/base/sidebar-custom-section.scss create mode 100644 db/migrate/20230307051200_add_position_to_sidebar_section_links.rb diff --git a/app/assets/javascripts/discourse/app/components/sidebar/section.hbs b/app/assets/javascripts/discourse/app/components/sidebar/section.hbs index fc2f5471ba6..11569ee5d0e 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/section.hbs +++ b/app/assets/javascripts/discourse/app/components/sidebar/section.hbs @@ -1,5 +1,12 @@ {{#if this.displaySection}} -
+
{{#each section.links as |link|}} {{#if link.external}} @@ -15,6 +16,12 @@ @prefixType="icon" @prefixValue={{link.icon}} @href={{link.value}} + @class={{link.linkDragCss}} + {{draggable + didStartDrag=link.didStartDrag + didEndDrag=link.didEndDrag + dragMove=link.dragMove + }} /> {{else}} {{/if}} {{/each}} diff --git a/app/assets/javascripts/discourse/app/components/sidebar/user/custom-sections.js b/app/assets/javascripts/discourse/app/components/sidebar/user/custom-sections.js index 96d69dffd14..3562777db9e 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/user/custom-sections.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/user/custom-sections.js @@ -1,12 +1,8 @@ import Component from "@glimmer/component"; -import showModal from "discourse/lib/show-modal"; import { inject as service } from "@ember/service"; -import RouteInfoHelper from "discourse/lib/sidebar/route-info-helper"; -import I18n from "I18n"; import { ajax } from "discourse/lib/ajax"; -import { iconHTML } from "discourse-common/lib/icon-library"; -import { htmlSafe } from "@ember/template"; import { bind } from "discourse-common/utils/decorators"; +import Section from "discourse/components/sidebar/user/section"; export default class SidebarUserCustomSections extends Component { @service currentUser; @@ -23,31 +19,13 @@ export default class SidebarUserCustomSections extends Component { } get sections() { - this.currentUser.sidebarSections.forEach((section) => { - if (!section.public || this.currentUser.staff) { - section.headerActions = [ - { - action: () => { - return showModal("sidebar-section-form", { model: section }); - }, - title: I18n.t("sidebar.sections.custom.edit"), - }, - ]; - } - section.decoratedTitle = - section.public && this.currentUser.staff - ? htmlSafe(`${iconHTML("globe")} ${section.title}`) - : section.title; - section.links.forEach((link) => { - if (!link.external) { - const routeInfoHelper = new RouteInfoHelper(this.router, link.value); - link.route = routeInfoHelper.route; - link.models = routeInfoHelper.models; - link.query = routeInfoHelper.query; - } + return this.currentUser.sidebarSections.map((section) => { + return new Section({ + section, + currentUser: this.currentUser, + router: this.router, }); }); - return this.currentUser.sidebarSections; } @bind diff --git a/app/assets/javascripts/discourse/app/components/sidebar/user/section-link.js b/app/assets/javascripts/discourse/app/components/sidebar/user/section-link.js new file mode 100644 index 00000000000..0cdce813f61 --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/sidebar/user/section-link.js @@ -0,0 +1,63 @@ +import { tracked } from "@glimmer/tracking"; +import { bind } from "discourse-common/utils/decorators"; +import RouteInfoHelper from "discourse/lib/sidebar/route-info-helper"; + +export default class SectionLink { + @tracked linkDragCss; + + constructor({ external, icon, id, name, value }, section, router) { + this.external = external; + this.icon = icon; + this.id = id; + this.name = name; + this.value = value; + this.section = section; + + if (!this.external) { + const routeInfoHelper = new RouteInfoHelper(router, value); + this.route = routeInfoHelper.route; + this.models = routeInfoHelper.models; + this.query = routeInfoHelper.query; + } + } + + @bind + didStartDrag(e) { + this.mouseY = e.targetTouches ? e.targetTouches[0].screenY : e.screenY; + } + + @bind + didEndDrag() { + this.linkDragCss = null; + this.mouseY = null; + this.section.enable(); + this.section.reorder(); + } + + @bind + dragMove(e) { + const currentMouseY = e.targetTouches + ? e.targetTouches[0].screenY + : e.screenY; + const distance = currentMouseY - this.mouseY; + if (!this.linkHeight) { + this.linkHeight = document.getElementsByClassName( + "sidebar-section-link-wrapper" + )[0].clientHeight; + } + if (distance >= this.linkHeight) { + if (this.section.links.indexOf(this) !== this.section.links.length - 1) { + this.section.moveLinkDown(this); + this.mouseY = currentMouseY; + } + } + if (distance <= -this.linkHeight) { + if (this.section.links.indexOf(this) !== 0) { + this.section.moveLinkUp(this); + this.mouseY = currentMouseY; + } + } + this.linkDragCss = "drag"; + this.section.disable(); + } +} diff --git a/app/assets/javascripts/discourse/app/components/sidebar/user/section.js b/app/assets/javascripts/discourse/app/components/sidebar/user/section.js new file mode 100644 index 00000000000..bafa6b37f59 --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/sidebar/user/section.js @@ -0,0 +1,79 @@ +import I18n from "I18n"; +import showModal from "discourse/lib/show-modal"; +import { iconHTML } from "discourse-common/lib/icon-library"; +import { htmlSafe } from "@ember/template"; +import SectionLink from "discourse/components/sidebar/user/section-link"; +import { tracked } from "@glimmer/tracking"; +import { bind } from "discourse-common/utils/decorators"; +import { ajax } from "discourse/lib/ajax"; + +export default class Section { + @tracked dragCss; + @tracked links; + + constructor({ section, currentUser, router }) { + this.section = section; + this.router = router; + this.currentUser = currentUser; + this.slug = section.slug; + + this.links = this.section.links.map((link) => { + return new SectionLink(link, this, this.router); + }); + } + + get decoratedTitle() { + return this.section.public && this.currentUser.staff + ? htmlSafe(`${iconHTML("globe")} ${this.section.title}`) + : this.section.title; + } + + get headerActions() { + if (!this.section.public || this.currentUser.staff) { + return [ + { + action: () => { + return showModal("sidebar-section-form", { model: this.section }); + }, + title: I18n.t("sidebar.sections.custom.edit"), + }, + ]; + } + } + + @bind + disable() { + this.dragCss = "disabled"; + } + + @bind + enable() { + this.dragCss = null; + } + + @bind + moveLinkDown(link) { + const position = this.links.indexOf(link) + 1; + this.links = this.links.removeObject(link); + this.links.splice(position, 0, link); + } + + @bind + moveLinkUp(link) { + const position = this.links.indexOf(link) - 1; + this.links = this.links.removeObject(link); + this.links.splice(position, 0, link); + } + @bind + reorder() { + return ajax(`/sidebar_sections/reorder`, { + type: "POST", + contentType: "application/json", + dataType: "json", + data: JSON.stringify({ + sidebar_section_id: this.section.id, + links_order: this.links.map((link) => link.id), + }), + }); + } +} diff --git a/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js b/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js index 6ec376db61a..59698abc4aa 100644 --- a/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js +++ b/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js @@ -172,7 +172,10 @@ export default Controller.extend(ModalFunctionality, { }), }) .then((data) => { - this.currentUser.sidebar_sections.pushObject(data.sidebar_section); + this.currentUser.set( + "sidebar_sections", + this.currentUser.sidebar_sections.concat(data.sidebar_section) + ); this.send("closeModal"); }) .catch((e) => diff --git a/app/assets/javascripts/discourse/app/modifiers/draggable.js b/app/assets/javascripts/discourse/app/modifiers/draggable.js index d2d6a8035b1..aca15eb024a 100644 --- a/app/assets/javascripts/discourse/app/modifiers/draggable.js +++ b/app/assets/javascripts/discourse/app/modifiers/draggable.js @@ -32,7 +32,7 @@ export default class DraggableModifier extends Modifier { this.hasStarted = true; if (this.didStartDragCallback) { - this.didStartDragCallback(); + this.didStartDragCallback(e); } // Register a global event to capture mouse moves when element 'clicked'. diff --git a/app/assets/stylesheets/common/base/_index.scss b/app/assets/stylesheets/common/base/_index.scss index eb810f8194f..e0cdea2837a 100644 --- a/app/assets/stylesheets/common/base/_index.scss +++ b/app/assets/stylesheets/common/base/_index.scss @@ -47,6 +47,7 @@ @import "share_link"; @import "shared-drafts"; @import "sidebar"; +@import "sidebar-custom-section"; @import "sidebar-footer"; @import "sidebar-section"; @import "sidebar-more-section-links"; diff --git a/app/assets/stylesheets/common/base/sidebar-custom-section.scss b/app/assets/stylesheets/common/base/sidebar-custom-section.scss new file mode 100644 index 00000000000..d34adc479a0 --- /dev/null +++ b/app/assets/stylesheets/common/base/sidebar-custom-section.scss @@ -0,0 +1,34 @@ +.sidebar-custom-sections { + .sidebar-section-wrapper { + padding-bottom: 0; + } + .d-icon-globe { + position: absolute; + left: 0.5em; + height: 0.75em; + width: 0.75em; + margin-top: 0.15em; + align-items: center; + } + + .sidebar-section-link-prefix.icon { + cursor: move; + } + + .sidebar-section-wrapper.disabled { + .sidebar-section-link-wrapper { + .sidebar-section-link-prefix.icon, + .sidebar-section-link { + background: none; + color: var(--primary-low); + } + .sidebar-section-link.drag { + font-weight: bold; + color: var(--primary-high); + .sidebar-section-link-prefix.icon { + color: var(--primary-high); + } + } + } + } +} diff --git a/app/assets/stylesheets/common/base/sidebar.scss b/app/assets/stylesheets/common/base/sidebar.scss index 1583db8b800..505db8a11a2 100644 --- a/app/assets/stylesheets/common/base/sidebar.scss +++ b/app/assets/stylesheets/common/base/sidebar.scss @@ -128,19 +128,6 @@ } } -.sidebar-custom-sections { - .sidebar-section-wrapper { - padding-bottom: 0; - } - .d-icon-globe { - position: absolute; - left: 0.5em; - height: 0.75em; - width: 0.75em; - margin-top: 0.15em; - align-items: center; - } -} .sidebar-section-form-modal { .modal-inner-container { width: var(--modal-max-width); diff --git a/app/controllers/sidebar_sections_controller.rb b/app/controllers/sidebar_sections_controller.rb index d5c10d3860a..0d811f554ba 100644 --- a/app/controllers/sidebar_sections_controller.rb +++ b/app/controllers/sidebar_sections_controller.rb @@ -56,6 +56,26 @@ class SidebarSectionsController < ApplicationController render json: failed_json, status: 403 end + def reorder + sidebar_section = SidebarSection.find_by(id: reorder_params["sidebar_section_id"]) + @guardian.ensure_can_edit!(sidebar_section) + + order = reorder_params["links_order"].map(&:to_i).each_with_index.to_h + position_generator = + (0..sidebar_section.sidebar_section_links.count * 2).excluding( + sidebar_section.sidebar_section_links.map(&:position), + ).each + links = + sidebar_section + .sidebar_section_links + .sort_by { |link| order[link.linkable_id] } + .map { |link| link.attributes.merge(position: position_generator.next) } + sidebar_section.sidebar_section_links.upsert_all(links, update_only: [:position]) + render json: sidebar_section + rescue Discourse::InvalidAccess + render json: failed_json, status: 403 + end + def destroy sidebar_section = SidebarSection.find_by(id: section_params["id"]) @guardian.ensure_can_delete!(sidebar_section) @@ -82,6 +102,10 @@ class SidebarSectionsController < ApplicationController params.permit(links: %i[icon name value id _destroy])["links"] end + def reorder_params + params.permit(:sidebar_section_id, links_order: []) + end + def check_if_member_of_group ### TODO remove when enable_custom_sidebar_sections SiteSetting is removed if !SiteSetting.enable_custom_sidebar_sections.present? || diff --git a/app/models/sidebar_section.rb b/app/models/sidebar_section.rb index 8b83a6d984e..e7b53480daf 100644 --- a/app/models/sidebar_section.rb +++ b/app/models/sidebar_section.rb @@ -2,7 +2,7 @@ class SidebarSection < ActiveRecord::Base belongs_to :user - has_many :sidebar_section_links, dependent: :destroy + has_many :sidebar_section_links, -> { order("position") }, dependent: :destroy has_many :sidebar_urls, through: :sidebar_section_links, source: :linkable, diff --git a/app/models/sidebar_section_link.rb b/app/models/sidebar_section_link.rb index c9a799b89d0..659ae8a8587 100644 --- a/app/models/sidebar_section_link.rb +++ b/app/models/sidebar_section_link.rb @@ -13,6 +13,12 @@ class SidebarSectionLink < ActiveRecord::Base SUPPORTED_LINKABLE_TYPES = %w[Category Tag SidebarUrl] before_validation { self.user_id ||= self.sidebar_section&.user_id } + before_create do + if self.user_id && self.sidebar_section + self.position = self.sidebar_section.sidebar_section_links.maximum(:position).to_i + 1 + end + end + after_destroy { self.linkable.destroy! if self.linkable_type == "SidebarUrl" } private def ensure_supported_linkable_type @@ -37,9 +43,11 @@ end # created_at :datetime not null # updated_at :datetime not null # sidebar_section_id :integer +# position :integer default(0), not null # # Indexes # # idx_unique_sidebar_section_links (user_id,linkable_type,linkable_id) UNIQUE # index_sidebar_section_links_on_linkable_type_and_linkable_id (linkable_type,linkable_id) +# links_user_id_section_id_position (user_id,sidebar_section_id,position) UNIQUE # diff --git a/config/routes.rb b/config/routes.rb index d1aa0b5b9b0..fb89b78901b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1594,6 +1594,7 @@ Discourse::Application.routes.draw do delete "user-status" => "user_status#clear" resources :sidebar_sections, only: %i[index create update destroy] + post "/sidebar_sections/reorder" => "sidebar_sections#reorder" get "*url", to: "permalinks#show", constraints: PermalinkConstraint.new end diff --git a/db/migrate/20230307051200_add_position_to_sidebar_section_links.rb b/db/migrate/20230307051200_add_position_to_sidebar_section_links.rb new file mode 100644 index 00000000000..a76c0f8d4fb --- /dev/null +++ b/db/migrate/20230307051200_add_position_to_sidebar_section_links.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class AddPositionToSidebarSectionLinks < ActiveRecord::Migration[7.0] + def change + add_column :sidebar_section_links, :position, :integer, default: 0, null: false + execute "UPDATE sidebar_section_links SET position = id" + add_index :sidebar_section_links, + %i[user_id sidebar_section_id position], + unique: true, + name: "links_user_id_section_id_position" + end +end diff --git a/spec/requests/sidebar_sections_controller_spec.rb b/spec/requests/sidebar_sections_controller_spec.rb index a975ecb4bb6..3ea42a95d42 100644 --- a/spec/requests/sidebar_sections_controller_spec.rb +++ b/spec/requests/sidebar_sections_controller_spec.rb @@ -167,6 +167,7 @@ RSpec.describe SidebarSectionsController do links: [ { icon: "link", id: sidebar_url_1.id, name: "latest", value: "/latest" }, { icon: "link", id: sidebar_url_2.id, name: "tags", value: "/tags", _destroy: "1" }, + { icon: "link", name: "homepage", value: "https://discourse.org" }, ], } @@ -178,10 +179,18 @@ RSpec.describe SidebarSectionsController do expect { section_link_2.reload }.to raise_error(ActiveRecord::RecordNotFound) expect { sidebar_url_2.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect(sidebar_section.sidebar_section_links.last.position).to eq(2) + expect(sidebar_section.sidebar_section_links.last.linkable.name).to eq("homepage") + expect(sidebar_section.sidebar_section_links.last.linkable.value).to eq( + "https://discourse.org", + ) + user_history = UserHistory.last expect(user_history.action).to eq(UserHistory.actions[:update_public_sidebar_section]) expect(user_history.subject).to eq("custom section edited") - expect(user_history.details).to eq("links: latest - /latest") + expect(user_history.details).to eq( + "links: latest - /latest, homepage - https://discourse.org", + ) end it "doesn't allow to edit other's sections" do @@ -232,6 +241,57 @@ RSpec.describe SidebarSectionsController do end end + describe "#reorder" do + fab!(:sidebar_section) { Fabricate(:sidebar_section, user: user) } + fab!(:sidebar_url_1) { Fabricate(:sidebar_url, name: "tags", value: "/tags") } + fab!(:sidebar_url_2) { Fabricate(:sidebar_url, name: "categories", value: "/categories") } + fab!(:sidebar_url_3) { Fabricate(:sidebar_url, name: "topic", value: "/t/1") } + fab!(:section_link_1) do + Fabricate(:sidebar_section_link, sidebar_section: sidebar_section, linkable: sidebar_url_1) + end + fab!(:section_link_2) do + Fabricate(:sidebar_section_link, sidebar_section: sidebar_section, linkable: sidebar_url_2) + end + fab!(:section_link_3) do + Fabricate(:sidebar_section_link, sidebar_section: sidebar_section, linkable: sidebar_url_3) + end + + it "sorts links" do + serializer = SidebarSectionSerializer.new(sidebar_section, root: false).as_json + expect(serializer[:links].map(&:id)).to eq( + [sidebar_url_1.id, sidebar_url_2.id, sidebar_url_3.id], + ) + + sign_in(user) + post "/sidebar_sections/reorder.json", + params: { + sidebar_section_id: sidebar_section.id, + links_order: [sidebar_url_2.id, sidebar_url_3.id, sidebar_url_1.id], + } + + serializer = SidebarSectionSerializer.new(sidebar_section.reload, root: false).as_json + expect(serializer[:links].map(&:id)).to eq( + [sidebar_url_2.id, sidebar_url_3.id, sidebar_url_1.id], + ) + end + + it "is not allowed for not own sections" do + sidebar_section_2 = Fabricate(:sidebar_section) + post "/sidebar_sections/reorder.json", + params: { + sidebar_section_id: sidebar_section_2.id, + links_order: [sidebar_url_2.id, sidebar_url_3.id, sidebar_url_1.id], + } + + expect(response.status).to eq(403) + + serializer = SidebarSectionSerializer.new(sidebar_section, root: false).as_json + expect(serializer[:links].map(&:id)).to eq( + [sidebar_url_1.id, sidebar_url_2.id, sidebar_url_3.id], + ) + end + end + describe "#destroy" do fab!(:sidebar_section) { Fabricate(:sidebar_section, user: user) } diff --git a/spec/system/custom_sidebar_sections_spec.rb b/spec/system/custom_sidebar_sections_spec.rb index 1926e91bc12..f0e944aafb6 100644 --- a/spec/system/custom_sidebar_sections_spec.rb +++ b/spec/system/custom_sidebar_sections_spec.rb @@ -80,6 +80,33 @@ describe "Custom sidebar sections", type: :system, js: true do expect(page).not_to have_link("Sidebar Categories") end + it "allows the user to reorder links in custom section" do + sidebar_section = Fabricate(:sidebar_section, title: "My section", user: user) + sidebar_url_1 = Fabricate(:sidebar_url, name: "Sidebar Tags", value: "/tags") + Fabricate(:sidebar_section_link, sidebar_section: sidebar_section, linkable: sidebar_url_1) + sidebar_url_2 = Fabricate(:sidebar_url, name: "Sidebar Categories", value: "/categories") + Fabricate(:sidebar_section_link, sidebar_section: sidebar_section, linkable: sidebar_url_2) + + visit("/latest") + within(".sidebar-custom-sections .sidebar-section-link-wrapper:nth-child(1)") do + expect(page).to have_css(".sidebar-section-link-sidebar-tags") + end + within(".sidebar-custom-sections .sidebar-section-link-wrapper:nth-child(2)") do + expect(page).to have_css(".sidebar-section-link-sidebar-categories") + end + + tags_link = find(".sidebar-section-link-sidebar-tags") + categories_link = find(".sidebar-section-link-sidebar-categories") + tags_link.drag_to(categories_link) + + within(".sidebar-custom-sections .sidebar-section-link-wrapper:nth-child(1)") do + expect(page).to have_css(".sidebar-section-link-sidebar-categories") + end + within(".sidebar-custom-sections .sidebar-section-link-wrapper:nth-child(2)") do + expect(page).to have_css(".sidebar-section-link-sidebar-tags") + end + end + it "does not allow the user to edit public section" do sidebar_section = Fabricate(:sidebar_section, title: "Public section", user: user, public: true) sidebar_url_1 = Fabricate(:sidebar_url, name: "Sidebar Tags", value: "/tags")