From 1c395e1a01b97080bc244ae2afd122be11778128 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Thu, 2 Nov 2023 08:46:45 +1100 Subject: [PATCH] FIX: reliably reorder link in custom sections (#24188) Two changes were introduced: 1. Reorder links on sidebar section is removed. Clicking and holding the mouse for 250ms was unintuitive; 2. Fixed bugs when reorder is done in edit modal. --- .../components/modal/sidebar-section-form.hbs | 2 + .../components/modal/sidebar-section-form.js | 38 ++++--- .../sidebar/common/custom-section.hbs | 18 ---- .../components/sidebar/section-form-link.js | 19 ++-- .../common/community-section/section.js | 1 - .../discourse/app/lib/sidebar/section-link.js | 101 ------------------ .../discourse/app/lib/sidebar/section.js | 2 - .../sidebar_sections_controller.rb | 11 -- config/routes.rb | 1 - .../sidebar_sections_controller_spec.rb | 66 ------------ spec/system/custom_sidebar_sections_spec.rb | 7 +- 11 files changed, 34 insertions(+), 232 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/modal/sidebar-section-form.hbs b/app/assets/javascripts/discourse/app/components/modal/sidebar-section-form.hbs index 4d9e3633913..7306a89683e 100644 --- a/app/assets/javascripts/discourse/app/components/modal/sidebar-section-form.hbs +++ b/app/assets/javascripts/discourse/app/components/modal/sidebar-section-form.hbs @@ -51,6 +51,7 @@ @link={{link}} @deleteLink={{this.deleteLink}} @reorderCallback={{this.reorder}} + @setDraggedLinkCallback={{this.setDraggedLink}} /> {{/each}} @@ -71,6 +72,7 @@ @link={{link}} @deleteLink={{this.deleteLink}} @reorderCallback={{this.reorder}} + @setDraggedLinkCallback={{this.setDraggedLink}} /> {{/each}} link.objectId === linkFromId - ); - if (!linkFrom) { - linkFrom = this.transformedModel.secondaryLinks.find( - (link) => link.objectId === linkFromId - ); - } - if (linkFrom.isPrimary) { - this.transformedModel.links.removeObject(linkFrom); + if (this.draggedLink.isPrimary) { + this.transformedModel.links.removeObject(this.draggedLink); } else { - this.transformedModel.secondaryLinks?.removeObject(linkFrom); + this.transformedModel.secondaryLinks?.removeObject(this.draggedLink); } - if (linkTo.isPrimary) { - const toPosition = this.transformedModel.links.indexOf(linkTo); - linkFrom.segment = "primary"; + if (targetLink.isPrimary) { + const toPosition = this.transformedModel.links.indexOf(targetLink); + this.draggedLink.segment = "primary"; this.transformedModel.links.insertAt( above ? toPosition : toPosition + 1, - linkFrom + this.draggedLink ); } else { - linkFrom.segment = "secondary"; - const toPosition = this.transformedModel.secondaryLinks.indexOf(linkTo); + this.draggedLink.segment = "secondary"; + const toPosition = + this.transformedModel.secondaryLinks.indexOf(targetLink); this.transformedModel.secondaryLinks.insertAt( above ? toPosition : toPosition + 1, - linkFrom + this.draggedLink ); } } diff --git a/app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.hbs b/app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.hbs index 0e895ca41c0..afe03c5e07f 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.hbs +++ b/app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.hbs @@ -19,15 +19,6 @@ @fullReload={{link.fullReload}} @href={{link.value}} @class={{link.linkDragCss}} - {{(if - this.section.reorderable - (modifier - "draggable" - didStartDrag=link.didStartDrag - didEndDrag=link.didEndDrag - dragMove=link.dragMove - ) - )}} /> {{else}} {{/if}} {{/each}} diff --git a/app/assets/javascripts/discourse/app/components/sidebar/section-form-link.js b/app/assets/javascripts/discourse/app/components/sidebar/section-form-link.js index 22a070bc280..5328d53ebbd 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/section-form-link.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/section-form-link.js @@ -1,6 +1,7 @@ import Component from "@glimmer/component"; import { tracked } from "@glimmer/tracking"; import { action } from "@ember/object"; +import discourseLater from "discourse-common/lib/later"; export default class SectionFormLink extends Component { @tracked dragCssClass; @@ -17,14 +18,14 @@ export default class SectionFormLink extends Component { @action dragHasStarted(event) { event.dataTransfer.effectAllowed = "move"; - event.dataTransfer.setData("linkId", this.args.link.objectId); + this.args.setDraggedLinkCallback(this.args.link); this.dragCssClass = "dragging"; } @action dragOver(event) { event.preventDefault(); - if (!this.dragCssClass) { + if (this.dragCssClass !== "dragging") { if (this.isAboveElement(event)) { this.dragCssClass = "drag-above"; } else { @@ -44,25 +45,23 @@ export default class SectionFormLink extends Component { this.dragCount === 0 && (this.dragCssClass === "drag-above" || this.dragCssClass === "drag-below") ) { - this.dragCssClass = null; + discourseLater(() => { + this.dragCssClass = null; + }, 10); } } @action dropItem(event) { event.stopPropagation(); - this.dragCounter = 0; - this.args.reorderCallback( - parseInt(event.dataTransfer.getData("linkId"), 10), - this.args.link, - this.isAboveElement(event) - ); + this.dragCount = 0; + this.args.reorderCallback(this.args.link, this.isAboveElement(event)); this.dragCssClass = null; } @action dragEnd() { - this.dragCounter = 0; + this.dragCount = 0; this.dragCssClass = null; } } diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/common/community-section/section.js b/app/assets/javascripts/discourse/app/lib/sidebar/common/community-section/section.js index 4b1e5067afd..b7fc31b7f6a 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/common/community-section/section.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/common/community-section/section.js @@ -41,7 +41,6 @@ export default class CommunitySection { @tracked links; @tracked moreLinks; - reorderable = false; hideSectionHeader = true; constructor({ section, owner }) { diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/section-link.js index e63d17ba474..0aad8be80e3 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/section-link.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/section-link.js @@ -1,11 +1,6 @@ import { tracked } from "@glimmer/tracking"; import RouteInfoHelper from "discourse/lib/sidebar/route-info-helper"; import { defaultHomepage } from "discourse/lib/utilities"; -import discourseLater from "discourse-common/lib/later"; -import { bind } from "discourse-common/utils/decorators"; - -const TOUCH_SCREEN_DELAY = 300; -const MOUSE_DELAY = 250; export default class SectionLink { @tracked linkDragCss; @@ -46,100 +41,4 @@ export default class SectionLink { get externalOrFullReload() { return this.external || this.fullReload || this.withAnchor; } - - @bind - didStartDrag(event) { - // 0 represents left button of the mouse - if (event.button === 0 || event.targetTouches) { - this.startMouseY = this.#calcMouseY(event); - this.willDrag = true; - - discourseLater( - () => { - this.delayedStart(event); - }, - event.targetTouches ? TOUCH_SCREEN_DELAY : MOUSE_DELAY - ); - } - } - - delayedStart(event) { - if (this.willDrag) { - const currentMouseY = this.#calcMouseY(event); - - if (currentMouseY === this.startMouseY) { - event.stopPropagation(); - event.preventDefault(); - this.mouseY = this.#calcMouseY(event); - this.linkDragCss = "drag"; - this.section.disable(); - this.drag = true; - } - } - } - - @bind - didEndDrag() { - this.linkDragCss = null; - this.mouseY = null; - this.section.enable(); - this.section.reorder(); - this.willDrag = false; - this.drag = false; - } - - @bind - dragMove(event) { - const moveMouseY = this.#calcMouseY(event); - - if (this.willDrag && moveMouseY !== this.startMouseY && !this.drag) { - /** - * If mouse position is different, it means that it is a scroll and not drag and drop action. - * In that case, we want to do nothing and keep original behaviour. - */ - this.willDrag = false; - return; - } else { - /** - * Otherwise, event propagation should be stopped as we have our own handler for drag and drop. - */ - event.stopPropagation(); - event.preventDefault(); - } - - this.startMouseY = moveMouseY; - - if (!this.drag) { - return; - } - - const currentMouseY = this.#calcMouseY(event); - 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; - } - } - } - - #calcMouseY(event) { - return Math.round( - event.targetTouches ? event.targetTouches[0].clientY : event.y - ); - } } diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/section.js b/app/assets/javascripts/discourse/app/lib/sidebar/section.js index 94c1b30f91a..7cb9ddd81b2 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/section.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/section.js @@ -15,8 +15,6 @@ export default class Section { @tracked dragCss; @tracked links; - reorderable = true; - constructor({ section, owner }) { setOwner(this, owner); diff --git a/app/controllers/sidebar_sections_controller.rb b/app/controllers/sidebar_sections_controller.rb index 5e84793d841..39b34f79dab 100644 --- a/app/controllers/sidebar_sections_controller.rb +++ b/app/controllers/sidebar_sections_controller.rb @@ -83,17 +83,6 @@ class SidebarSectionsController < ApplicationController render_serialized(sidebar_section, SidebarSectionSerializer) 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 - set_order(sidebar_section, order) - - render_serialized(sidebar_section, SidebarSectionSerializer) - 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) diff --git a/config/routes.rb b/config/routes.rb index 530be0fd1cf..267ca86dad3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1607,7 +1607,6 @@ 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" put "/sidebar_sections/reset/:id" => "sidebar_sections#reset" get "*url", to: "permalinks#show", constraints: PermalinkConstraint.new diff --git a/spec/requests/sidebar_sections_controller_spec.rb b/spec/requests/sidebar_sections_controller_spec.rb index a8a0bb3764a..bfe8371b584 100644 --- a/spec/requests/sidebar_sections_controller_spec.rb +++ b/spec/requests/sidebar_sections_controller_spec.rb @@ -370,72 +370,6 @@ RSpec.describe SidebarSectionsController do end end - describe "#reorder" do - fab!(:user2) { Fabricate(:user) } - 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 - expect(sidebar_section.sidebar_urls.pluck(: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], - } - - expect(response.status).to eq(200) - - expect(sidebar_section.reload.sidebar_urls.pluck(:id)).to eq( - [sidebar_url_2.id, sidebar_url_3.id, sidebar_url_1.id], - ) - end - - it "returns 403 when a user tries to reorder a section that doesn't belong to them" do - sign_in(user2) - - 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], - } - - expect(response.status).to eq(403) - - expect(sidebar_section.reload.sidebar_urls.pluck(:id)).to eq( - [sidebar_url_1.id, sidebar_url_2.id, sidebar_url_3.id], - ) - end - - it "returns 403 for an non user" do - 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], - } - - expect(response.status).to eql(403) - 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 9427657cf69..802d7966a3a 100644 --- a/spec/system/custom_sidebar_sections_spec.rb +++ b/spec/system/custom_sidebar_sections_spec.rb @@ -178,9 +178,12 @@ describe "Custom sidebar sections", type: :system do ["Sidebar Tags", "Sidebar Categories", "Sidebar Latest"], ) - tags_link = find(".sidebar-section-link[data-link-name='Sidebar Tags']") - latest_link = find(".sidebar-section-link[data-link-name='Sidebar Latest']") + sidebar.edit_custom_section("My section") + + tags_link = find(".draggable[data-link-name='Sidebar Tags']") + latest_link = find(".draggable[data-link-name='Sidebar Latest']") tags_link.drag_to(latest_link, html5: true, delay: 0.4) + section_modal.save expect(sidebar.primary_section_links("my-section")).to eq( ["Sidebar Categories", "Sidebar Tags", "Sidebar Latest"],