Pin stream (formerly "star stream") #819

kartikmaji · 2016-05-23T19:26:13Z

Star favourite or important streams.
Starred streams will be sorted at the top of Left side stream list.

@neerajwahi

smarx · 2016-05-23T19:26:14Z

Automated message from Dropbox CLA bot

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

timabbott · 2016-05-24T21:06:16Z

Looks like this is failing tests? Regardless @neerajwahi @acrefoot can one of you take a look?

neerajwahi · 2016-05-25T08:54:41Z

@krtkmj taking a look at this, in the meantime please fix the failing tests.

kartikmaji · 2016-05-25T21:29:31Z

@neerajwahi all tests passing now.

neerajwahi · 2016-06-02T02:56:00Z

@krtkmj There are some bugs here. For instance, try starring stream Verona in your dev VM and notice that the threads under Verona are always visible despite switching away to another stream.

I also noticed an intermittent issue where the threads underneath a starred stream get doubled up (look at Verona below):

kartikmaji · 2016-06-02T10:33:15Z

@neerajwahi I guess the purpose of starring stream is that it remains open irrespective of narrowed to any other stream. I also mentioned that in commit message.

And, I am looking for doubled topic list.

neerajwahi · 2016-06-05T18:02:21Z

@timabbott The current behavior of this diff is to keep the stream uncollapsed (and the list of threads showing) when a stream is starred regardless of whether that stream is selected. There's discussion around it here: #292

What are your thoughts on this behavior? I personally think it's confusing and makes the starred stream section too cluttered. I initially thought it was a bug until @krtkmj pointed out that it was by design.

timabbott · 2016-06-05T18:53:18Z

I think the intent of the current feature is just to sort the pinned streams at the top of the list of streams, and not to keep them always expanded; I agree that we should not combine this feature with the idea in #292. I think #292 probably makes sense to be an independent setting if we were to add that feature, since some users may want one and the not the other behavior..

kartikmaji · 2016-06-05T19:25:27Z

@timabbott @neerajwahi shall we have another button which will be responsible for keeping the stream open? This won't take much time and can add this feature in this pr itself.

timabbott · 2016-06-05T19:36:14Z

No, I'd prefer to have that be feature idea be a separate PR and discussion -- let's get this working completely correctly as described, so we can merge it.

As a general rule, it's usually easier to merge multiple features one at a time.

kartikmaji · 2016-06-06T18:57:39Z

@neerajwahi updated the pull request. Please review.

neerajwahi · 2016-06-08T00:58:13Z

@krtkmj I think we should remove the settings icon next to the 'Starred' section. It doesn't do what I expect -- control only starred streams -- and the functionality is currently duplicated across both sections.

@timabbott Do you think that starring a stream should remove it from the 'Streams' section, or duplicate it into both sections (acting as more of a shortcut)?

timabbott · 2016-06-08T01:01:49Z

Hmm, interesting question. I could see good reason to do it either way. I'd probably start with having it remove it from the "Streams" section, since I think that may be more intuitive, and we can always change that based on feedback.

timabbott · 2016-06-08T01:02:48Z

(Also, I think that will be easier to implement, because with the alternative, we'd have to deal with only doing the topics list expansion in one of the two copies if the user narrows to the stream).

neerajwahi · 2016-06-08T02:23:46Z

static/js/stream_list.js

@@ -153,14 +184,15 @@ exports.set_in_home_view = function (stream, in_home) {
    }
 };

-function build_stream_sidebar_row(name) {
+function build_stream_sidebar_row (name) {


You added an inadvertent space here. It's a good habit to go through and review the diff yourself before pushing. I usually just run git diff in the console and look through it before I create a PR

neerajwahi · 2016-06-08T02:32:10Z

@krtkmj Added some inline comments

kartikmaji · 2016-06-08T12:16:01Z

@timabbott I agree with @neerajwahi but removing settings icon next to "STREAM" may seem like we don't have a settings page for normal streams. Or we can have something like this

timabbott · 2016-06-08T16:34:18Z

@krtkmj I'm not sure what you mean by "removing settings icon next to stream" but the design in that screenshot is what I had mind for showing the streams themselves.

[edit] Oh, maybe what you're asking is where to put the settings gear? Good question! I would probably just put it next to STARRED and not include ALL STREAMS.

kartikmaji · 2016-06-08T16:50:11Z

@timabbott I guess settings gear just in front of starred stream might be confusing as @neerajwahi commented earlier.
My suggestion is to (referring screenshot posted above) change ALL STREAMS to SUBSCRIBED STREAMS and let settings gear in that same line because clicking that simply opens up "Manage Streams" page. What do you think?

timabbott · 2016-06-11T00:02:04Z

That seems reasonable to me.

timabbott · 2016-06-11T18:46:00Z

After thinking about this more, I have a a new proposals for how to do the display:

We retain the existing "STREAMS" heading, sort the starred teams to the top, and don't add any other new headings.
We use a horizontal line to delimit the pinned streams from the rest of the streams.
We rename the feature from "Star stream" to "pin stream to stop" (would called the variable on the Subscription object ".pinned").

What do you think @krtkmj and @neerajwahi? I think this would be a clean way to implement this feature without cluttering the UI.

kartikmaji · 2016-06-11T20:45:41Z

I think this will make more sense along with cleaner UI.
But, why change the feature name? Because, "pin stream to stop" may sound like stopping incoming messages.

kartikmaji · 2016-06-11T22:06:51Z

@timabbott This is how it looks now. Pretty decent now. Any suggestion?
cc @neerajwahi

timabbott · 2016-06-12T01:50:55Z

That looks pretty reasonable to me at a high level; I think we could potentially make it nicer by changing the styling (e.g. maybe there's too 2x much space above/below the horizontal line?).

timabbott · 2016-06-28T21:25:05Z

static/js/stream_list.js

+        // If no stream filter box is there,
+        // return all streams.
+        return streams;
+    }


Is this change needed? Seems like it is kinda its own change related to the stream filter box functionality.

In file frontend_tests.node_tests.stream_list.js, function test_sort_pin_to_top_streams does not add .stream-list-filter to body. Hence, search_box.expectOne() in line number 31 fails.
So, I added this code if no filter box is detected then return streams without filtering.
Or I can just append .stream-list-filter in body and remove this part of code. Thoughts?

If this is just for the Node tests, yeah, I'd just create this in the Node test code...

@krtkmj looks like you haven't addressed this in your latest update?

Looks like this is now resolved.

timabbott · 2016-06-28T21:29:42Z

Cool, I posted a few comments. Once those are resolved, I think this should be ready to merge from a code perspective. It would be great to clean up the history for this. What I'd love to see is individual commits for the changes whose purpose as part of the "pin streams" feature isn't clear (most of those I posted a question about in my comments above, e.g. some of the CSS and test framework changes), and then rest of the changes squashed together as a single large commit that adds this feature.

Does that make sense @krtkmj?

kartikmaji · 2016-06-29T11:23:04Z

@timabbott updated this PR. Have some doubts that I posted in inline comments.

…en clicked.

… streams by horizontal line.

…ice-versa.

neerajwahi · 2016-06-30T02:45:54Z

@krtkmj Added some inline comments

timabbott · 2016-06-30T23:50:20Z

@krtkmj can you fix your git author name to be "Kartik Maji", not your username?

timabbott · 2016-06-30T23:54:24Z

I merged the subscriber list sorting as a32167d.

timabbott · 2016-06-30T23:54:54Z

static/templates/stream_sidebar_actions.handlebars

+      {{else}}
+      {{t Pin to top }}
+      {{/if}}
+      stream <b>{{stream.name}}</b>


word stream here should also be tagged for translations.

timabbott · 2016-07-01T00:20:34Z

Noticed a few other display bugs while trying to merge this; since I need to head home I'm posting my working branch.
https://github.com/timabbott/zulip/commits/starred
Will plan to finish cleaning this up and merge it soon.

timabbott · 2016-07-01T05:28:49Z

OK, that ended up being all I needed to do (and in fact, the CSS piece was no longer needed), so I merged this. Huge thanks @krtkmj and @laurenzlong for building this!!!

laurenzlong · 2016-07-01T15:50:59Z

Awesome!! Thanks for all your support Tim!

On Thu, Jun 30, 2016, 10:28 PM Tim Abbott notifications@github.com wrote:

Closed #819 #819.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#819 (comment), or mute the
thread
https://github.com/notifications/unsubscribe/ADzlFJhywnyf4cPUHgHoMV72FPRycxyaks5qRKWYgaJpZM4IkzLt
.

Lauren

kartikmaji force-pushed the star-stream branch 2 times, most recently from 587e7cb to e64761d Compare May 25, 2016 21:09

kartikmaji force-pushed the star-stream branch from e64761d to 9545f69 Compare June 6, 2016 18:56

neerajwahi reviewed Jun 8, 2016
View reviewed changes

kartikmaji force-pushed the star-stream branch from 9545f69 to 9eb2345 Compare June 11, 2016 23:01

timabbott reviewed Jun 28, 2016
View reviewed changes

kartikmaji force-pushed the star-stream branch from 0414161 to cec2630 Compare June 29, 2016 11:22

laurenzlong and others added 13 commits June 29, 2016 17:04

Add starred field to Subscription model.

95a84fb

Add popover option for starring stream, and communicates to server wh…

c3adb33

…en clicked.

starred streams now show up at top of sidebar and add test.

ecfc636

revise UI for starred stream

51f4fb0

add ability to star/unstar stream in main subscription settings page

74e530b

Add front-end test for starred stream feature.

d53c3a1

revise left sidebar design to have 2 headers: streams and starred

7a0c290

Update stream list when starred stream. Split starred and non starred…

d623ed8

… streams by horizontal line.

Change feature name star stream to pin to top stream.

bb2e432

Tag pin to top keywords in stream popover for translation.

10752f5

Hide stream list titles and pinned stream splitter when zoom_in and v…

d87eef3

…ice-versa.

Center push pin icon and align text in stream popover.

788bdae

Add type annotation in test_change_pin_stream test.

6305f8d

Add subscription properties test for stream's pin to top property.

0c2c7f6

kartikmaji force-pushed the star-stream branch from cec2630 to 0c2c7f6 Compare June 30, 2016 15:04

timabbott reviewed Jun 30, 2016
View reviewed changes

timabbott closed this Jul 1, 2016

Pin stream (formerly "star stream") #819

Pin stream (formerly "star stream") #819

Conversation

kartikmaji commented May 23, 2016 • edited Loading

smarx commented May 23, 2016

timabbott commented May 24, 2016

neerajwahi commented May 25, 2016

kartikmaji commented May 25, 2016

neerajwahi commented Jun 2, 2016 • edited Loading

kartikmaji commented Jun 2, 2016

neerajwahi commented Jun 5, 2016

timabbott commented Jun 5, 2016

kartikmaji commented Jun 5, 2016

timabbott commented Jun 5, 2016

kartikmaji commented Jun 6, 2016

neerajwahi commented Jun 8, 2016

timabbott commented Jun 8, 2016

timabbott commented Jun 8, 2016 • edited Loading

neerajwahi Jun 8, 2016

Choose a reason for hiding this comment

neerajwahi commented Jun 8, 2016

kartikmaji commented Jun 8, 2016 • edited Loading

timabbott commented Jun 8, 2016 • edited Loading

kartikmaji commented Jun 8, 2016

timabbott commented Jun 11, 2016

timabbott commented Jun 11, 2016

kartikmaji commented Jun 11, 2016

kartikmaji commented Jun 11, 2016

timabbott commented Jun 12, 2016

timabbott Jun 28, 2016

Choose a reason for hiding this comment

kartikmaji Jun 29, 2016

Choose a reason for hiding this comment

timabbott Jun 29, 2016

Choose a reason for hiding this comment

neerajwahi Jun 30, 2016

Choose a reason for hiding this comment

timabbott Jul 1, 2016

Choose a reason for hiding this comment

timabbott commented Jun 28, 2016

kartikmaji commented Jun 29, 2016

neerajwahi commented Jun 30, 2016

timabbott commented Jun 30, 2016

timabbott commented Jun 30, 2016

timabbott Jun 30, 2016 • edited Loading

Choose a reason for hiding this comment

timabbott commented Jul 1, 2016

timabbott commented Jul 1, 2016

laurenzlong commented Jul 1, 2016

kartikmaji commented May 23, 2016 •

edited

Loading

neerajwahi commented Jun 2, 2016 •

edited

Loading

timabbott commented Jun 8, 2016 •

edited

Loading

kartikmaji commented Jun 8, 2016 •

edited

Loading

timabbott commented Jun 8, 2016 •

edited

Loading

timabbott Jun 30, 2016 •

edited

Loading