Skip to content

Commit

Permalink
FIX: consistent sidebar section external links (discourse#22343)
Browse files Browse the repository at this point in the history
Before this change, links which required full reload because they are not in ember routes like `/my/preferences` or links to docs like `/pub/*` were treated as real external links. Therefore, they were opening in self window or new tab based on user  `external_links_in_new_tab` setting.

To be consistent with behavior when full reload links are in the post, they are treated as internal and always open in the same window.
  • Loading branch information
lis2 authored Jun 30, 2023
1 parent 898e571 commit 3c019b1
Show file tree
Hide file tree
Showing 7 changed files with 37 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@
@class={{this.section.dragCss}}
>
{{#each this.section.links as |link|}}
{{#if link.external}}
{{#if link.externalOrFullReload}}
<Sidebar::SectionLink
@shouldDisplay={{link.shouldDisplay}}
@linkName={{link.name}}
@content={{replace-emoji link.text}}
@prefixType="icon"
@prefixValue={{link.prefixValue}}
@fullReload={{link.fullReload}}
@href={{link.value}}
@class={{link.linkDragCss}}
{{(if
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
{{#if @sectionLink.external}}
{{#if @sectionLink.externalOrFullReload}}
<Sidebar::SectionLink
@shouldDisplay={{@sectionLink.shouldDisplay}}
@linkName={{@sectionLink.name}}
@content={{replace-emoji @sectionLink.text}}
@prefixType="icon"
@prefixValue={{@sectionLink.prefixValue}}
@fullReload={{@sectionLink.fullReload}}
@href={{@sectionLink.value}}
/>
{{else}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ export default class SectionLink extends Component {
}

get target() {
if (this.args.fullReload) {
return "_self";
}
return this.currentUser?.user_option?.external_links_in_new_tab
? "_blank"
: "_self";
Expand Down
13 changes: 11 additions & 2 deletions app/assets/javascripts/discourse/app/lib/sidebar/section-link.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,21 @@ const MOUSE_DELAY = 250;
export default class SectionLink {
@tracked linkDragCss;

constructor({ external, icon, id, name, value }, section, router) {
constructor(
{ external, full_reload, icon, id, name, value },
section,
router
) {
this.external = external;
this.fullReload = full_reload;
this.prefixValue = icon;
this.id = id;
this.name = name;
this.text = name;
this.value = value;
this.section = section;

if (!this.external) {
if (!this.externalOrFullReload) {
const routeInfoHelper = new RouteInfoHelper(router, value);
this.route = routeInfoHelper.route;
this.models = routeInfoHelper.models;
Expand All @@ -30,6 +35,10 @@ export default class SectionLink {
return true;
}

get externalOrFullReload() {
return this.external || this.fullReload;
}

@bind
didStartDrag(event) {
// 0 represents left button of the mouse
Expand Down
8 changes: 6 additions & 2 deletions app/serializers/sidebar_url_serializer.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
# frozen_string_literal: true

class SidebarUrlSerializer < ApplicationSerializer
attributes :id, :name, :value, :icon, :external, :segment
attributes :id, :name, :value, :icon, :external, :full_reload, :segment

def external
object.external? || object.full_reload?
object.external?
end

def full_reload
object.full_reload?
end
end
12 changes: 9 additions & 3 deletions spec/system/custom_sidebar_sections_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
let(:section_modal) { PageObjects::Modals::SidebarSectionForm.new }
let(:sidebar) { PageObjects::Components::Sidebar.new }

before { user.user_option.update!(external_links_in_new_tab: true) }

it "allows the user to create custom section" do
visit("/latest")

Expand Down Expand Up @@ -40,7 +42,7 @@
section_modal.save

expect(sidebar).to have_section("My section")
expect(sidebar).to have_section_link("My preferences")
expect(sidebar).to have_section_link("My preferences", target: "_self")
end

it "allows the user to create custom section with /pub link" do
Expand All @@ -53,7 +55,7 @@
section_modal.save

expect(sidebar).to have_section("My section")
expect(sidebar).to have_section_link("Published Page")
expect(sidebar).to have_section_link("Published Page", target: "_self")
end

it "allows the user to create custom section with external link" do
Expand All @@ -76,7 +78,11 @@
section_modal.save

expect(sidebar).to have_section("My section")
expect(sidebar).to have_section_link("Discourse Homepage", href: "https://discourse.org")
expect(sidebar).to have_section_link(
"Discourse Homepage",
href: "https://discourse.org",
target: "_blank",
)
end

it "allows the user to edit custom section" do
Expand Down
7 changes: 4 additions & 3 deletions spec/system/page_objects/components/sidebar.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ def has_one_active_section_link?
has_css?(".#{SIDEBAR_SECTION_LINK_SELECTOR}--active", count: 1)
end

def has_section_link?(name, href: nil, active: false)
section_link_present?(name, href: href, active: active, present: true)
def has_section_link?(name, href: nil, active: false, target: nil)
section_link_present?(name, href: href, active: active, target: target, present: true)
end

def has_no_section_link?(name, href: nil, active: false)
Expand Down Expand Up @@ -119,11 +119,12 @@ def primary_section_icons(slug)

private

def section_link_present?(name, href: nil, active: false, present:)
def section_link_present?(name, href: nil, active: false, target: nil, present:)
attributes = { exact_text: name }
attributes[:href] = href if href
attributes[:class] = SIDEBAR_SECTION_LINK_SELECTOR
attributes[:class] += "--active" if active
attributes[:target] = target if target
page.public_send(present ? :has_link? : :has_no_link?, **attributes)
end

Expand Down

0 comments on commit 3c019b1

Please sign in to comment.