zulip-desktop icon indicating copy to clipboard operation
zulip-desktop copied to clipboard

Published 20 hours ago • verified publisher icon zulip

Reorder sidebar org list by dragging

Open abhigyank opened this issue 4 years ago • 6 comments

Rebase and update PR #617 to master.

Feature works as expected, tested upto 5 organizations.

I haven't gone through the code and logic yet, and would soon do so, but I would encourage you to test the feature as is now and report in usability issues and mishappenings.

abhigyank avatar Jan 06 '21 16:01 abhigyank

@akashnimare is it possible for you test this out ?

abhigyank avatar Jan 06 '21 16:01 abhigyank

The code logic looks fine to me and the feature is working. The PR can be reviewed and ready for merging if approved.

cc @andersk @timabbott .

abhigyank avatar Jan 07 '21 16:01 abhigyank

Awesome, thanks for rebasing @abhigyank! @andersk do you have time to review this? I think it's one of the top few features users want in the desktop app.

timabbott avatar Jan 23 '21 00:01 timabbott

We have far too many indices flying around to be confident that it would be correct to start mutating them:

  • ServerManagerView#activeTabIndex
  • ServerManagerView#tabIndex
  • ServerTab#index
  • ServerTab#tabIndex
  • SettingsOptions.lastActiveTab
  • WebView#index
  • WebView#tabIndex
  • the data-tab-id attribute
  • the index closed by ServerTab#onHoverOut
  • the index closed by ServerTab#onHover
  • the index closed by WebView#isActive
  • the indices of DomainUtil.getDomains()
  • the indices of ServerManagerView#tabs
  • the indices of document.querySelectorAll(".server-icons")
  • the indices of document.querySelectorAll(".tab .server-icons")
  • the indices of document.querySelectorAll(".tab .server-tooltip")
  • the values of ServerManagerView#functionalTabs

Before I’ll accept any PR that allows organizations to be reordered, I want as many of these indices as possible to be replaced by stable identifiers that do not change under reordering.

(A good example of a stable identifier is the reference to the Tab object itself. We could probably solve many of these problems by storing direct object references to the Tab instead of indirect numerical references to one of its indices.)

andersk avatar Apr 03 '21 23:04 andersk

Heads up @abhigyank, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot avatar Nov 24 '21 02:11 zulipbot

Is this merged? i would like to take on the issue @akashnimare @abhigyank

varun-s22 avatar Jan 19 '23 14:01 varun-s22