Make org tabs draggable in sidebar #617

kanishk98 · 2018-12-23T17:30:32Z

What's this PR do?
Updates domains.json on dragging server tabs and (intends to) reorder server tooltips along with that.

Any background context you want to provide?
Please refer to the discussion in #548.

You have tested this PR on:

Windows
Linux/Ubuntu
macOS

Current behaviour on Windows 10 (CSS animations by sortablejs not recorded by Windows Game Bar)

Problems in current version
On my computer, the server tab freezes for a bit before I can switch tabs again.

Other possible solutions that I considered

We could just change the server names if we're able to also map the hoverable area of a server tab properly.
We could swap the tooltips' nodes in the DOM (they're just sibling nodes, we should be able to replaceChild() as long as we maintain clones of the old tooltip node.

abhigyank · 2018-12-23T18:37:00Z

Don't know if its because its in WIP but the webviews (incll sidebar) hang for me if I try to drag and change the order,

kanishk98 · 2018-12-24T08:50:34Z

Don't know if its because its in WIP but the webviews (incll sidebar) hang for me if I try to drag and change the order,

I don't know why that's happening. Can you explain any specific steps you took so that I can reproduce this?
I've updated the main PR description with a GIF showing sidebar behaviour on my end.

kanishk98 · 2018-12-24T08:56:30Z

I've found another problem that you may see if you clear app data, add a couple (or more) organisations and then drag them around:

This is caused because of line number 178 in app/renderer/js/main.js, as follows:
domains.push(DomainUtil.getDomain(oldIndex));

I'll need to get the actual old index from the domains.json file instead of just taking it from the DOM. Will get to that in some time.

abhigyank · 2018-12-24T16:34:46Z

@kanishk98 attached a gif. You can also see the console error for onHover which probably is causing this. If you wamt I'd provide the full traceback or debug it for you.

kanishk98 · 2018-12-24T19:09:29Z

@abhigyank thanks for the gif. Looks like this.state has not been properly defined. I'll get to it.

kanishk98 · 2019-01-02T04:36:53Z

As for the problem I mentioned above (related to clearing app data, dragging new domains, and getting a crash), that's because the data-tab-id attribute only increments from the index of the previously added organisation. Can I get some ideas on where to start with this bug?

abhigyank · 2019-01-06T06:32:12Z

Yes, that is the main issue here. From what I recall, Akash had previously suggested storing the index of the servers in domain.json itself. Then we'd be work all functions according to that index.

kanishk98 · 2019-01-08T09:56:31Z

Yes, that is the main issue here. From what I recall, Akash had previously suggested storing the index of the servers in domain.json itself. Then we'd be work all functions according to that index.

I've used another solution, as follows:

tabElements.forEach((el, index) => {
	const oldIndex = +Number(el.getAttribute('data-tab-id')) % tabElements.length;

This should work fine for the reset and drag crash.

kanishk98 · 2019-01-12T17:06:11Z

I'm unable to understand why the webviews freeze after dragging the server tabs. I've logged that they load and the index of the registered clicks is correct (and corresponds to the correct server). I'd really like some pointers on why this might happen.

kanishk98 · 2019-01-21T20:34:28Z

To keep maintainers updated on the status of this PR, I've attached a gif below.

I think this achieves the required functionality. Just working on a couple of things to make sure minimum code changes are needed. :)

akashnimare · 2019-01-22T04:28:10Z

Looks great 👍

kanishk98 · 2019-02-18T19:51:21Z

Hello, just checking in to ask if any other changes are required here. :)

Edit: I realized a little too late that running git rebase was the wrong idea here. The commit history is a little messed up on my end (duplicated commits) so if the maintainers give me the go ahead here, I'll clean up git history on my local machine and send in another PR.
Sorry, this work is from quite a while ago. :)

app/package.json

kanishk98 · 2019-07-30T16:33:28Z

Updated PR to TypeScript.

akashnimare · 2019-08-13T20:22:59Z

@kanishk98 please update the PR. @priyank-p can you help him with the implementation/logic here?

kanishk98 · 2019-08-14T11:02:53Z

please update the PR.

Fixed merge conflicts - I think the functionality was mostly alright. Would love a review anytime. :)

priyank-p

Left some comments! Looks like you forgot to add sortablejs as a dependency?

priyank-p · 2019-01-22T12:40:10Z

app/renderer/js/main.js

@@ -156,12 +156,12 @@ class ServerManagerView {
 				const domains = [];
 				const tabElements = document.querySelectorAll('#tabs-container .tab');
 				tabElements.forEach((el, index) => {
-					const oldIndex = +el.getAttribute('data-tab-id');
+					const oldIndex = +Number(el.getAttribute('data-tab-id'));


The use of Number is useless here, that's what the + does here.

priyank-p · 2019-07-30T17:03:22Z

app/renderer/main.html

@@ -15,7 +15,7 @@
            </div>
            <div id="sidebar" class="toggle-sidebar">
                <div id="view-controls-container">
-                    <div id="tabs-container"></div>
+                    <div id="tabs-container" class="simple-list"></div>


We might want to pick a better name for this class here like organizations-list or sortable-organizations-list here.

priyank-p · 2019-07-30T17:08:26Z

app/renderer/js/main.ts

@@ -231,14 +233,37 @@ class ServerManagerView {
 		}
 	}

+	onEnd(): void {
+		const newServers: any[] | ServerOrFunctionalTab[] = [];
+		const tabMap: any = {};


You should create a interface here for this, it's just:

Suggested change

const tabMap: any = {};

interface TabMap {

[key: number]: number;

}

const tabMap: TabMap = {};

Also we should probably add a detailed comment here what tabMap stores and how it works. I used know the logic back almost a year ago and now looking at this I have no idea what this is which is the perfect reason to add a comment here.

priyank-p · 2019-08-14T17:09:23Z

app/renderer/js/main.ts

@@ -123,6 +126,7 @@ class ServerManagerView {
 		this.$dndTooltip = $actionsContainer.querySelector('#dnd-tooltip');

 		this.$sidebar = document.querySelector('#sidebar');
+		this.$drag = document.querySelector('#simple-list');


I think we could be more verbose here i.e. this.$sortableDrag

priyank-p · 2019-08-14T17:10:15Z

app/renderer/js/main.ts

@@ -94,7 +94,7 @@ class ServerManagerView {
 	$fullscreenEscapeKey: string;
 	loading: AnyObject;
 	activeTabIndex: number;
-	servers: ServerOrFunctionalTab[];
+	servers: any[];


why is this switched back to any?

priyank-p · 2019-08-14T17:11:56Z

app/main/index.ts

@@ -366,7 +366,9 @@ app.on('ready', () => {

 app.on('before-quit', () => {
 	mainWindow.webContents.send('save-domains');
-	isQuitting = true;
+	setTimeout(() => {
+		isQuitting = true;


Hmm, how did you come to this conclusion here? This seems a workaround than a correct fix.

kanishk98 · 2019-08-20T11:28:24Z

@priyank-p thanks a lot for the review! I think a couple of those mistakes you listed above are because I accidentally force pushed a faulty commit at the time of review, but I'll have a look again and finalize this PR by tonight. :)

kanishk98 · 2019-08-20T16:35:25Z

I've updated this PR and used a batch-write function for replacing the old order of domains with the new order. This works well for me, I'd love for everyone to try it out on their end and check for missing servers/duplication (this is why we're reading the order of servers from memory for the most part).

akashnimare · 2019-09-19T20:35:38Z

@kanishk98 can you fix the merge conflict? Also, I have noticed that we are not loading the server which is being dragged and I think we want to load that server instead of the new org on that index. Also, the order is not right -

https://cl.ly/8d7c41fba8dc

zulipbot · 2019-11-13T09:30:54Z

Heads up @kanishk98, 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/master branch and resolve your pull request's merge conflicts accordingly.

manavmehta · 2020-03-24T06:50:02Z

@akashnimare I can't reproduce this

Also, the order is not right

The order is working well for me
@abhigyank can you have a look please ?

akashnimare · 2020-03-24T09:40:04Z

I would suggest we should first look into the DB migration and then work on this feature.

abhigyank · 2021-01-06T18:53:39Z

Due to no recent activity, closing in favour of #1059 .

kanishk98 force-pushed the org-tab-drag branch 2 times, most recently from 4b970aa to f23cd95 Compare December 23, 2018 17:53

akashnimare requested review from abhigyank and priyank-p December 23, 2018 18:15

zulipbot added the size: XL label Jan 1, 2019

kanishk98 mentioned this pull request Jan 2, 2019

Fix flashing icons of servers in sidebar #621

Merged

3 tasks

zulipbot added size: M and removed size: XL labels Jan 8, 2019

kanishk98 force-pushed the org-tab-drag branch 4 times, most recently from b58c34e to b9e600b Compare January 8, 2019 09:47

zulipbot added size: L and removed size: M labels Jan 9, 2019

kanishk98 force-pushed the org-tab-drag branch from eb129f3 to d3764e2 Compare January 10, 2019 07:05

kanishk98 force-pushed the org-tab-drag branch from 171ff4b to 7774da8 Compare January 22, 2019 19:40

kanishk98 changed the title ~~[WIP] Make org tabs draggable in sidebar~~ Make org tabs draggable in sidebar Feb 3, 2019

akashnimare requested changes Mar 25, 2019

View reviewed changes

app/package.json Outdated Show resolved Hide resolved

zulipbot added the size: L label May 18, 2019

kanishk98 force-pushed the org-tab-drag branch 3 times, most recently from 6386a39 to 98f6fc3 Compare June 23, 2019 19:27

kanishk98 force-pushed the org-tab-drag branch from 98f6fc3 to f96767a Compare July 30, 2019 16:33

kanishk98 force-pushed the org-tab-drag branch from f96767a to 20272a2 Compare August 14, 2019 11:01

zulipbot added size: M and removed size: L labels Aug 14, 2019

priyank-p reviewed Aug 14, 2019

View reviewed changes

sidebar: Add sortablejs for reordering orgs.

a6dd47d

kanishk98 added 2 commits August 20, 2019 19:49

sidebar: Update domains on tab drag.

b879825

domains: Batch-update domains on tab drag.

98f40de

kanishk98 force-pushed the org-tab-drag branch from 20272a2 to 98f40de Compare August 20, 2019 16:32

zulipbot added size: L and removed size: M labels Aug 20, 2019

zulipbot added the has conflicts label Nov 13, 2019

abhigyank mentioned this pull request Mar 3, 2020

Make the org tab list draggable in left sidebar #548

Open

abhigyank added a commit to abhigyank/zulip-desktop that referenced this pull request Jan 6, 2021

Rebase zulip#617 with master and update to project's current code style.

d77eaed

abhigyank mentioned this pull request Jan 6, 2021

Reorder sidebar org list by dragging #1059

Open

abhigyank closed this Jan 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make org tabs draggable in sidebar #617

Make org tabs draggable in sidebar #617

kanishk98 commented Dec 23, 2018 •

edited

Loading

abhigyank commented Dec 23, 2018

kanishk98 commented Dec 24, 2018

kanishk98 commented Dec 24, 2018

abhigyank commented Dec 24, 2018

kanishk98 commented Dec 24, 2018

kanishk98 commented Jan 2, 2019

abhigyank commented Jan 6, 2019

kanishk98 commented Jan 8, 2019 •

edited

Loading

kanishk98 commented Jan 12, 2019

kanishk98 commented Jan 21, 2019

akashnimare commented Jan 22, 2019

kanishk98 commented Feb 18, 2019 •

edited

Loading

kanishk98 commented Jul 30, 2019

akashnimare commented Aug 13, 2019

kanishk98 commented Aug 14, 2019

priyank-p left a comment

priyank-p Jan 22, 2019

priyank-p Jul 30, 2019

priyank-p Jul 30, 2019

priyank-p Jul 30, 2019

priyank-p Aug 14, 2019

priyank-p Aug 14, 2019

priyank-p Aug 14, 2019

kanishk98 commented Aug 20, 2019

kanishk98 commented Aug 20, 2019

akashnimare commented Sep 19, 2019

zulipbot commented Nov 13, 2019

manavmehta commented Mar 24, 2020 •

edited

Loading

akashnimare commented Mar 24, 2020

abhigyank commented Jan 6, 2021 •

edited

Loading

-		const tabMap: any = {};
+interface TabMap {
+    [key: number]: number;
+}
+const tabMap: TabMap = {};

Make org tabs draggable in sidebar #617

Make org tabs draggable in sidebar #617

Conversation

kanishk98 commented Dec 23, 2018 • edited Loading

abhigyank commented Dec 23, 2018

kanishk98 commented Dec 24, 2018

kanishk98 commented Dec 24, 2018

abhigyank commented Dec 24, 2018

kanishk98 commented Dec 24, 2018

kanishk98 commented Jan 2, 2019

abhigyank commented Jan 6, 2019

kanishk98 commented Jan 8, 2019 • edited Loading

kanishk98 commented Jan 12, 2019

kanishk98 commented Jan 21, 2019

akashnimare commented Jan 22, 2019

kanishk98 commented Feb 18, 2019 • edited Loading

kanishk98 commented Jul 30, 2019

akashnimare commented Aug 13, 2019

kanishk98 commented Aug 14, 2019

priyank-p left a comment

Choose a reason for hiding this comment

priyank-p Jan 22, 2019

Choose a reason for hiding this comment

priyank-p Jul 30, 2019

Choose a reason for hiding this comment

priyank-p Jul 30, 2019

Choose a reason for hiding this comment

priyank-p Jul 30, 2019

Choose a reason for hiding this comment

priyank-p Aug 14, 2019

Choose a reason for hiding this comment

priyank-p Aug 14, 2019

Choose a reason for hiding this comment

priyank-p Aug 14, 2019

Choose a reason for hiding this comment

kanishk98 commented Aug 20, 2019

kanishk98 commented Aug 20, 2019

akashnimare commented Sep 19, 2019

zulipbot commented Nov 13, 2019

manavmehta commented Mar 24, 2020 • edited Loading

akashnimare commented Mar 24, 2020

abhigyank commented Jan 6, 2021 • edited Loading

kanishk98 commented Dec 23, 2018 •

edited

Loading

kanishk98 commented Jan 8, 2019 •

edited

Loading

kanishk98 commented Feb 18, 2019 •

edited

Loading

manavmehta commented Mar 24, 2020 •

edited

Loading

abhigyank commented Jan 6, 2021 •

edited

Loading