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

Avoid opening the compose box for sidebar clicks. #4499

Closed
wants to merge 1 commit into from

Conversation

showell
Copy link
Contributor

@showell showell commented Apr 14, 2017

We no longer automatically open the compose box for sidebar
actions. The logic behind this is that most users spend more time
reading messages than composing messages, and opening the compose
box can interfere with efficient browsing. On a mobile device, the
compose box takes up a significant chunk of your screen real estate.
On a bigger device, the compose box intercepts important navigation
keys like page-up and end, and even when it allows them, it can be
jarring.

This change definitely introduces a tradeoff, as there will be
slightly more friction for users to compose new messages. My
judgment is that we have plenty of other options available:

  • hotkeys: enter, r, c, C
  • clicking on the message you're replying to
  • click on "New topic"
  • click on "New private message"
  • use stream chevron send-message option
  • use topic chevron send-message option
  • use buddy list chevron send-message option
  • use message info menu option
  • use buddy list search and hit enter for the first user
    in the search results (I think we should change this too)

This commit also simplifies our code, since it's purely removing
code. If we want to re-introduce auto-open capability, we will
probably want to use an approach that is more testable. The logic
that was removed here was part of a function that is 200+ lines in
length, so it was never unit tested. It also seemed to have a logic
flaw in terms of assuming that the else clause for the
narrowed_to_topic() test implied we were dealing with private
messages. That may have been true in the context it was originally
written, but it was a ticking time bomb, because what if you are
just narrowed to a stream?

Fixes #3886

We no longer automatically open the compose box for sidebar
actions.  The logic behind this is that most users spend more time
reading messages than composing messages, and opening the compose
box can interfere with efficient browsing.  On a mobile device, the
compose box takes up a significant chunk of your screen real estate.
On a bigger device, the compose box intercepts important navigation
keys like page-up and end, and even when it allows them, it can be
jarring.

This change definitely introduces a tradeoff, as there will be
slightly more friction for users to compose new messages.  My
judgment is that we have plenty of other options available:

- hotkeys: enter, r, c, C
- clicking on the message you're replying to
- click on "New topic"
- click on "New private message"
- use stream chevron send-message option
- use topic chevron send-message option
- use buddy list chevron send-message option
- use message info menu option
- use buddy list search and hit enter for the first user
  in the search results (I think we should change this too)

This commit also simplifies our code, since it's purely removing
code.  If we want to re-introduce auto-open capability, we will
probably want to use an approach that is more testable.  The logic
that was removed here was part of a function that is 200+ lines in
length, so it was never unit tested. It also seemed to have a logic
flaw in terms of assuming that the `else` clause for the
`narrowed_to_topic()` test implied we were dealing with private
messages.  That may have been true in the context it was originally
written, but it was a ticking time bomb, because what if you are
just narrowed to a stream?

Fixes zulip#3886
@zulipbot
Copy link
Member

Hello @zulip/server-sidebars members, this pull request references an issue with the "area: left-sidebar" label, so you may want to check it out!

@smarx
Copy link

smarx commented Apr 14, 2017

Automated message from Dropbox CLA bot

@showell, it looks like you've already signed the Dropbox CLA. Thanks!

@lonerz
Copy link
Member

lonerz commented Apr 14, 2017

@showell I initially made these changes in #3960, but @timabbott thought otherwise. Nonetheless, I agree with your changes.

@showell
Copy link
Contributor Author

showell commented Apr 14, 2017

@lonerz Ha, I didn't realize that! I think we're obviously on the same page. :)

Here was some related conversation on chat: https://chat.zulip.org/#narrow/near/188034/stream/frontend/topic/unit.20testing.20compose.2Estart

@lonerz
Copy link
Member

lonerz commented Apr 14, 2017

Closing my PR then.

@timabbott
Copy link
Sponsor Member

Reading the code, I think there was no "ticking time bomb", because exports.narrowed_by_reply ensures the narrow was a topic or pm-with narrow.

@timabbott
Copy link
Sponsor Member

timabbott commented Apr 14, 2017

So I think removing the topic code path here is probably the right way to fix that issue. I'm much more skeptical of removing the pm-with code path -- I think users have a fairly strong expectation that clicking on a user in the right sidebar gets them in a position to "just type" to send a message to that user.

I think there's a good reason to have asymmetric behavior here -- the reading-to-writing ratio for PMs is generally close to 1, whereas for stream messages it is generally much more than 10 (depending on the size of the stream, etc.).

@showell
Copy link
Contributor Author

showell commented Apr 14, 2017

Ok, we should probably re-open this discussion on chat. I think the code on lines 216-219 has no good reason to be 150+ lines away from the code on lines 378-383 that Josh and I removed, so we could probably combine them and extract a method called narrow.activate_compose that we can put some tests around.

One data point on PMs is that currently, when you click on the upper left sidebar, it doesn't open the compose box. It's interesting to me that nobody's really complained about that. But I also find your asymmetry argument somewhat persuasive, so I'd be inclined to say that any narrowing action related to PMs should open the compose box, whereas no narrowing actions related to streams should open the compose box. (In streams mode, you'd click on buttons/messages/hotkeys once you're in the narrow, but not as part of a narrowing action.)

@showell
Copy link
Contributor Author

showell commented Apr 16, 2017

Abandoning this PR, based on latest decisions. I'm working on a more involved PR that addresses the compose box a bit more deeply.

@showell showell closed this Apr 16, 2017
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.

None yet

5 participants