fix(sidebar): render nav items as real links so middle-click works - #3601
Open
itsdaveit wants to merge 1 commit into
Open
fix(sidebar): render nav items as real links so middle-click works#3601itsdaveit wants to merge 1 commit into
itsdaveit wants to merge 1 commit into
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
Contributor
Confidence Score: 5/5Safe to merge — the change is narrow, purely additive, and has no effect on visual output or existing click behaviour. Only one file changed. Navigation now happens through the router link instead of router.push inside the click handler, but the outcome is identical. Items without a to prop continue rendering as buttons. No logic paths are removed that weren't replaced by an equivalent mechanism. Files Needing Attention: No files require special attention. Reviews (2): Last reviewed commit: "fix(sidebar): render nav items as real l..." | Re-trigger Greptile |
Sidebar items were rendered as <button> because AppSidebar never passed a `to` prop, even though every navigable item already knew its route target and frappe-ui's SidebarItem renders a RouterLink whenever `to` is present. As a result none of the sidebar entries could be opened in a new tab — middle-click, ctrl/cmd-click and "open link in new tab" all did nothing, and the items were not exposed as links to assistive technology. Pass the route target through as `to` for nav items, saved views and the mobile notifications entry. Navigation is then handled by the router link itself, so selectItem() only keeps its side effects (immediate highlight and the caller's hook). Items without a route target — search and the notifications toggle — keep rendering as buttons.
itsdaveit
force-pushed
the
fix/sidebar-nav-links
branch
from
July 28, 2026 23:11
8eae226 to
995f625
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Sidebar entries cannot be opened in a new tab. Middle-click, ctrl/cmd-click and the browser's "open link in new tab" all do nothing, because every item renders as a
<button>— there is no link target for the browser to act on. The entries are also not exposed as links to assistive technology.Cause
frappe-ui'sSidebarItemalready renders aRouterLinkwhenever atoprop is present and only falls back to<button>otherwise.AppSidebarnever passesto, although every navigable item already knows its route target — it just used it inside the click handler (router.push).Change
Pass the route target through as
tofor the three navigable item groups: nav items, saved views and the mobile notifications entry. Navigation is then performed by the router link itself, soselectItem()keeps only its side effects (setting the active item immediately, before the route settles, plus the caller's hook).Items that have no route target — search and the notifications toggle — are unchanged and keep rendering as buttons.
Notes
SidebarItemrenders identical markup in both branches.activeItemis still set eagerly for instant highlighting, and the existing route watcher keeps it in sync.useRouter/RouteLocationRawimports.Testing
Verified in a production Helpdesk instance: middle-click, ctrl-click and the context menu now open tickets, customers, contacts and saved views in a new tab; regular clicks and active-state highlighting are unaffected.