From ac7ec426b0482deb61b93558e0f33539b79bb972 Mon Sep 17 00:00:00 2001 From: Roland Crosby Date: Tue, 31 Mar 2020 11:13:01 -0400 Subject: [PATCH] Add stream sorting widget to subscriptions page This change adds a toggle widget to the "add streams" page that lets the user change the sort order of the streams list. So far, this supports sorting by stream name, by number of subscribers, or by estimated weekly traffic. --- frontend_tests/node_tests/subs.js | 77 +++++++++++++++++++++++++++++++ static/js/stream_data.js | 42 +++++++++++++++-- static/js/subs.js | 34 ++++++++++++-- static/styles/subscriptions.scss | 4 ++ 4 files changed, 151 insertions(+), 6 deletions(-) diff --git a/frontend_tests/node_tests/subs.js b/frontend_tests/node_tests/subs.js index 02c8d2fab9..2f6ba38613 100644 --- a/frontend_tests/node_tests/subs.js +++ b/frontend_tests/node_tests/subs.js @@ -38,6 +38,8 @@ run_test('filter_table', () => { name: 'Denmark', stream_id: 1, description: 'Copenhagen', + subscribers: {size: 1}, + is_old_stream: false, }, { elem: 'poland', @@ -45,6 +47,9 @@ run_test('filter_table', () => { name: 'Poland', stream_id: 2, description: 'monday', + subscribers: {size: 3}, + is_old_stream: true, + stream_weekly_traffic: 13, }, { elem: 'pomona', @@ -52,6 +57,9 @@ run_test('filter_table', () => { name: 'Pomona', stream_id: 3, description: 'college', + subscribers: {size: 0}, + is_old_stream: true, + stream_weekly_traffic: 0, }, { elem: 'cpp', @@ -59,6 +67,19 @@ run_test('filter_table', () => { name: 'C++', stream_id: 4, description: 'programming lang', + subscribers: {size: 2}, + is_old_stream: true, + stream_weekly_traffic: 6, + }, + { + elem: 'zzyzx', + subscribed: true, + name: 'Zzyzx', + stream_id: 5, + description: 'california town', + subscribers: {size: 2}, + is_old_stream: true, + stream_weekly_traffic: 6, }, ]; @@ -117,6 +138,7 @@ run_test('filter_table', () => { assert(!$(".stream-row-poland").hasClass("notdisplayed")); assert(!$(".stream-row-pomona").hasClass("notdisplayed")); assert($(".stream-row-cpp").hasClass("notdisplayed")); + assert($(".stream-row-zzyzx").hasClass("notdisplayed")); // assert these once and call it done assert(ui_called); @@ -126,6 +148,7 @@ run_test('filter_table', () => { '.stream-row-poland', '.stream-row-pomona', '.stream-row-cpp', + '.stream-row-zzyzx', '.stream-row-denmark', ]); @@ -135,12 +158,14 @@ run_test('filter_table', () => { assert(!$(".stream-row-poland").hasClass("notdisplayed")); assert($(".stream-row-pomona").hasClass("notdisplayed")); assert($(".stream-row-cpp").hasClass("notdisplayed")); + assert($(".stream-row-zzyzx").hasClass("notdisplayed")); subs.filter_table({input: "Den, Pol", subscribed_only: false}); assert(!$(".stream-row-denmark").hasClass("notdisplayed")); assert(!$(".stream-row-poland").hasClass("notdisplayed")); assert($(".stream-row-pomona").hasClass("notdisplayed")); assert($(".stream-row-cpp").hasClass("notdisplayed")); + assert($(".stream-row-zzyzx").hasClass("notdisplayed")); // Search is case-insensitive subs.filter_table({input: "po", subscribed_only: false}); @@ -148,6 +173,7 @@ run_test('filter_table', () => { assert(!$(".stream-row-poland").hasClass("notdisplayed")); assert(!$(".stream-row-pomona").hasClass("notdisplayed")); assert($(".stream-row-cpp").hasClass("notdisplayed")); + assert($(".stream-row-zzyzx").hasClass("notdisplayed")); // Search handles unusual characters like C++ subs.filter_table({input: "c++", subscribed_only: false}); @@ -155,6 +181,7 @@ run_test('filter_table', () => { assert($(".stream-row-poland").hasClass("notdisplayed")); assert($(".stream-row-pomona").hasClass("notdisplayed")); assert(!$(".stream-row-cpp").hasClass("notdisplayed")); + assert($(".stream-row-zzyzx").hasClass("notdisplayed")); // Search subscribed streams only subs.filter_table({input: "d", subscribed_only: true}); @@ -162,6 +189,7 @@ run_test('filter_table', () => { assert(!$(".stream-row-poland").hasClass("notdisplayed")); assert($(".stream-row-pomona").hasClass("notdisplayed")); assert($(".stream-row-cpp").hasClass("notdisplayed")); + assert($(".stream-row-zzyzx").hasClass("notdisplayed")); // Search terms match stream description subs.filter_table({input: "Co", subscribed_only: false}); @@ -169,6 +197,7 @@ run_test('filter_table', () => { assert($(".stream-row-poland").hasClass("notdisplayed")); assert(!$(".stream-row-pomona").hasClass("notdisplayed")); assert($(".stream-row-cpp").hasClass("notdisplayed")); + assert($(".stream-row-zzyzx").hasClass("notdisplayed")); // Search names AND descriptions sub_table_append = []; @@ -178,10 +207,56 @@ run_test('filter_table', () => { assert(!$(".stream-row-poland").hasClass("notdisplayed")); assert(!$(".stream-row-pomona").hasClass("notdisplayed")); assert($(".stream-row-cpp").hasClass("notdisplayed")); + assert($(".stream-row-zzyzx").hasClass("notdisplayed")); assert.deepEqual(sub_table_append, [ '.stream-row-pomona', '.stream-row-poland', '.stream-row-cpp', + '.stream-row-zzyzx', + '.stream-row-denmark', + ]); + + // Explicitly order streams by name + sub_table_append = []; + subs.filter_table({input: "", subscribed_only: false, sort_order: "by-stream-name"}); + assert.deepEqual(sub_table_append, [ + '.stream-row-cpp', + '.stream-row-denmark', + '.stream-row-poland', + '.stream-row-pomona', + '.stream-row-zzyzx', + ]); + + // Order streams by subscriber count + sub_table_append = []; + subs.filter_table({input: "", subscribed_only: false, sort_order: "by-subscriber-count"}); + assert.deepEqual(sub_table_append, [ + '.stream-row-poland', + '.stream-row-cpp', + '.stream-row-zzyzx', + '.stream-row-denmark', + '.stream-row-pomona', + ]); + + // Order streams by weekly traffic + sub_table_append = []; + subs.filter_table({input: "", subscribed_only: false, sort_order: "by-weekly-traffic"}); + assert.deepEqual(sub_table_append, [ + '.stream-row-poland', + '.stream-row-cpp', + '.stream-row-zzyzx', + '.stream-row-pomona', + '.stream-row-denmark', + ]); + + // Showing subscribed streams only puts un-subscribed DOM elements last + sub_table_append = []; + subs.filter_table({input: "", subscribed_only: true, sort_order: "by-subscriber-count"}); + assert.deepEqual(sub_table_append, [ + '.stream-row-poland', + '.stream-row-cpp', + '.stream-row-zzyzx', + '.stream-row-pomona', '.stream-row-denmark', ]); @@ -206,12 +281,14 @@ run_test('filter_table', () => { assert(!$(".stream-row-poland").hasClass("notdisplayed")); assert($(".stream-row-pomona").hasClass("notdisplayed")); assert($(".stream-row-cpp").hasClass("notdisplayed")); + assert($(".stream-row-zzyzx").hasClass("notdisplayed")); subs.filter_table({input: "d", subscribed_only: true}); assert($(".stream-row-denmark").hasClass("notdisplayed")); assert(!$(".stream-row-poland").hasClass("notdisplayed")); assert($(".stream-row-pomona").hasClass("notdisplayed")); assert($(".stream-row-cpp").hasClass("notdisplayed")); + assert($(".stream-row-zzyzx").hasClass("notdisplayed")); // test selected row set to active $(".stream-row[data-stream-id='1']").removeClass("active"); diff --git a/static/js/stream_data.js b/static/js/stream_data.js index 9e76c7cf1a..8cd9223458 100644 --- a/static/js/stream_data.js +++ b/static/js/stream_data.js @@ -795,7 +795,7 @@ exports.get_streams_for_settings_page = function () { return all_subs; }; -exports.sort_for_stream_settings = function (stream_ids) { +exports.sort_for_stream_settings = function (stream_ids, order) { // TODO: We may want to simply use util.strcmp here, // which uses Intl.Collator() when possible. @@ -807,13 +807,49 @@ exports.sort_for_stream_settings = function (stream_ids) { return sub.name.toLocaleLowerCase(); } - function by_stream_name(id_a, id_b) { + function weekly_traffic(stream_id) { + const sub = exports.get_sub_by_id(stream_id); + if (sub && sub.is_old_stream) { + return sub.stream_weekly_traffic; + } + // don't intersperse new streams with zero-traffic existing streams + return -1; + } + + function by_stream_name(id_a, id_b) { const stream_a_name = name(id_a); const stream_b_name = name(id_b); return String.prototype.localeCompare.call(stream_a_name, stream_b_name); } - stream_ids.sort(by_stream_name); + function by_subscriber_count(id_a, id_b) { + const out = exports.get_sub_by_id(id_b).subscribers.size + - exports.get_sub_by_id(id_a).subscribers.size; + if (out === 0) { + return by_stream_name(id_a, id_b); + } + return out; + } + + function by_weekly_traffic(id_a, id_b) { + const out = weekly_traffic(id_b) - weekly_traffic(id_a); + if (out === 0) { + return by_stream_name(id_a, id_b); + } + return out; + } + + const orders = { + "by-stream-name": by_stream_name, + "by-subscriber-count": by_subscriber_count, + "by-weekly-traffic": by_weekly_traffic, + }; + + if (typeof order === "undefined" || !orders.hasOwnProperty(order)) { + order = "by-stream-name"; + } + + stream_ids.sort(orders[order]); }; exports.get_streams_for_admin = function () { diff --git a/static/js/subs.js b/static/js/subs.js index ccfd74537f..383630a9d5 100644 --- a/static/js/subs.js +++ b/static/js/subs.js @@ -371,8 +371,8 @@ function get_stream_id_buckets(stream_ids, query) { } } - stream_data.sort_for_stream_settings(buckets.name); - stream_data.sort_for_stream_settings(buckets.desc); + stream_data.sort_for_stream_settings(buckets.name, query.sort_order); + stream_data.sort_for_stream_settings(buckets.desc, query.sort_order); return buckets; } @@ -392,7 +392,7 @@ exports.populate_stream_settings_left_panel = function () { }; // query is now an object rather than a string. -// Query { input: String, subscribed_only: Boolean } +// Query { input: String, subscribed_only: Boolean, sort_order: String } exports.filter_table = function (query) { exports.show_active_stream_in_left_panel(); @@ -454,6 +454,7 @@ exports.filter_table = function (query) { }; let subscribed_only = true; +let sort_order = "by-stream-name"; exports.get_search_params = function () { const search_box = $("#stream_filter input[type='text']"); @@ -461,6 +462,7 @@ exports.get_search_params = function () { const params = { input: input, subscribed_only: subscribed_only, + sort_order: sort_order, }; return params; }; @@ -500,6 +502,18 @@ exports.switch_stream_tab = function (tab_name) { stream_edit.setup_subscriptions_tab_hash(tab_name); }; +exports.sort_toggler = undefined; +exports.switch_stream_sort = function (tab_name) { + if (tab_name === "by-stream-name" + || tab_name === "by-subscriber-count" + || tab_name === "by-weekly-traffic") { + sort_order = tab_name; + } else { + sort_order = "by-stream-name"; + } + exports.actually_filter_streams(); +}; + exports.setup_page = function (callback) { // We should strongly consider only setting up the page once, // but I am writing these comments write before a big release, @@ -515,6 +529,20 @@ exports.setup_page = function (callback) { // continue the strategy that we re-render everything from scratch. // Also, we'll always go back to the "Subscribed" tab. function initialize_components() { + // Sort by name by default when opening "Manage streams". + sort_order = "by-stream-name"; + exports.sort_toggler = components.toggle({ + values: [ + { label: ``, key: "by-stream-name" }, + { label: ``, key: "by-subscriber-count" }, + { label: ``, key: "by-weekly-traffic" }, + ], + callback: function (value, key) { + exports.switch_stream_sort(key); + }, + }); + $("#subscriptions_table .search-container").prepend(exports.sort_toggler.get()); + // Reset our internal state to reflect that we're initially in // the "Subscribed" tab if we're reopening "Manage streams". subscribed_only = true; diff --git a/static/styles/subscriptions.scss b/static/styles/subscriptions.scss index ea52f3fbcf..7a59bb8035 100644 --- a/static/styles/subscriptions.scss +++ b/static/styles/subscriptions.scss @@ -399,6 +399,10 @@ form#add_new_subscription { max-width: 1200px; max-height: 1000px; + .search-container .tab-switcher .ind-tab { + width: auto; + } + .subscriptions-header { padding: 12px; text-align: center;