Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorder sidebar org list by dragging #1059

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

abhigyank
Copy link
Collaborator

@abhigyank abhigyank commented Jan 6, 2021

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
Copy link
Collaborator Author

@akashnimare is it possible for you test this out ?

@abhigyank abhigyank changed the title Org tab drag Reorder sidebar org list by dragging Jan 6, 2021
@abhigyank abhigyank mentioned this pull request Jan 6, 2021
3 tasks
@abhigyank
Copy link
Collaborator Author

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 .

Base automatically changed from master to main January 22, 2021 19:22
@timabbott
Copy link
Sponsor Member

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.

@andersk
Copy link
Member

andersk commented Apr 3, 2021

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.)

@zulipbot
Copy link
Member

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.

@varun-s22
Copy link
Collaborator

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

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

Successfully merging this pull request may close these issues.

None yet

6 participants