Skip to content

fix: remove duplicate sidebar header trigger#2842

Open
Vaibhaav-Tiwari wants to merge 2 commits into
mainfrom
fix/sidebar-header-trigger
Open

fix: remove duplicate sidebar header trigger#2842
Vaibhaav-Tiwari wants to merge 2 commits into
mainfrom
fix/sidebar-header-trigger

Conversation

@Vaibhaav-Tiwari

Copy link
Copy Markdown
Collaborator

Summary

  • remove the duplicate expanded-state sidebar collapse button from the header
  • keep the collapsed sidebar expand trigger and edge rail behavior intact
  • add a regression test that the expanded header does not render the duplicate collapse trigger

Tests

  • npm.cmd run test -- Sidebar.test.tsx
  • npm.cmd run typecheck

@codebanditssss

Copy link
Copy Markdown
Collaborator

thanks for this, but i think it's gone stale and the premise no longer holds, so holding off on approving.

the branch is ~106 commits behind main and currently conflicting. more importantly, the sidebar header was refactored on main since this was cut (the shell-unification work: #2937, #2974, #3055, etc.). it's no longer two separate static triggers, it's a single adaptive one:

{!isMac && !isWindows && (
  <SidebarTrigger aria-label={isCollapsed ? "Expand sidebar" : "Collapse sidebar"} ... />
)}

so there isn't a duplicate to remove anymore, it's one control that flips label/behavior by state (and it's linux-only in the header now, mac uses the titlebar). the new test also asserts that no "Collapse sidebar" button renders, but on current main that's the legitimate collapse control in the expanded linux sidebar, so the assertion is backwards vs main and removing it would leave linux users with only ⌘B / drag to collapse.

could you rebase on latest main and re-check whether any duplicate actually still exists? my read is it's already been consolidated and this is likely obsolete, but if you spot a real dupe after rebasing, happy to look at a fresh diff against the new single-trigger structure. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants