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

deselect conversation *FREEBIE* #1904

Closed
wants to merge 2 commits into from
Closed

deselect conversation *FREEBIE* #1904

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 17, 2017

First time contributor checklist

Contributor checklist

  • My contribution is fully baked and ready to be merged as is
  • My changes are rebased on the latest master branch
  • My commits are in nice logical chunks
  • I have followed the best practices in my commit messages
  • I have tested my contribution on these platforms:
  • macOS 10.13.2 (17C88) - standalone
  • My changes pass all the local tests 100%
  • I have considered whether my changes need additional tests, and in the case they do, I have written them

Description

This introduced the feature requested in #1784
Now it's possible to deselect the conversation by pressing Shift + ESC.
In order to do that, now the conversations are not stacked anymore, but when a new conversation is opened, the DIV of the previous one is removed. This should also help in case of a very large number of conversations opened, but it requires some additional time to load the messages when a new conversation is opened.

@scottnonnenberg
Copy link
Contributor

Hm. I'm not sure about this. It would introduce a pretty substantial regression in most users' eyes, but it would reduce memory usage.

One effect we'd definitely need to address if we're going to do this is that every time a conversation was re-opened, we'd load another 100 messages back into history. Maybe the onOpened handler in ConversationView should check to see how many messages are already loaded and only fetch if necessary?

@ghost
Copy link
Author

ghost commented Dec 18, 2017

Agree with you. Currently converstation_stack works well keeping all the opened conversations into different DIVs. I can try to implement your solution this weekend, if it's ok for you.

@scottnonnenberg
Copy link
Contributor

Let's leave it at this: this is an interesting experiment, and we'll play with it once it's implemented, and only then decide whether to merge it.

It may be that re-rendering the conversation is reasonably fast (as opposed to both message fetch and render), but we'll have to make the call once we've spent some time with it.

@ghost
Copy link
Author

ghost commented Dec 19, 2017

In order to leave the current stack, now it just prepends the conversation-placeholder so all the opened conversations will still be there.

@scottnonnenberg what do you think about? To me this is the optimal conservative solution, the only thing you may want to check is the shortcut. I am not sure whether users would be happy to have only a shortcut or they may want also a "close conversation" icon. But for the time beings, it can be more than enough.

@scottnonnenberg
Copy link
Contributor

Okay, played with it a little bit. I like where it's going, but I think, for it to be intuitive, it needs two changes:

  1. When you close a conversation, it should be unloaded completely. There's a simple change for that, in conversation_view.js:
this.listenTo(this.model, 'closed', this.onClosed);
...
onClosed: function() {
  this.unload('user close');
},
  1. When you close a conversation, you should see the last conversation you had open. And you should only see the welcome screen if you have no conversations open at all.

For this, I'm thinking that InboxView.closeConversation() should not think about which conversation is open at all. It should simply call this.conversation_stack.close(), which will look at what's at the top of the stack, pull the conversation id out of the DOM element id, and then pull that from ConversationController, trigger 'closed' on that Conversation, then figure out what's next on the stack. If it's the load screen, then put ourselves in an 'unselected' state. If there's a conversation at the top, then do the work to make that conversation fully selected. Then we won't be trying to synchronize state with the DOM - we trust the stack of DOM elements in the stack.

@scottnonnenberg-signal
Copy link
Contributor

Closing this for now; big changes will be required to get this merged.

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

Successfully merging this pull request may close these issues.

None yet

2 participants