From ebb389ef8ae23c8cfb3f0a6ea57fb0afd8b90454 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Thu, 29 Aug 2019 12:03:43 -0300 Subject: [PATCH] UX: Read indicator improvements. (#8049) * The read indicator now shows up when no member has read the last post of the topic (written by a non-member) * The read indicator works on mobile and receives live updates from message bus * The icon we display in the topic list was changed * Added a title to the indicator to indicate its purpose when hovering over it --- .../components/basic-topic-list.js.es6 | 45 +++++++++++++++++++ .../components/topic-list-item.js.es6 | 28 ++++++------ .../templates/list/topic-list-item.raw.hbs | 6 +-- .../templates/list/unread-indicator.raw.hbs | 5 +++ .../mobile/components/basic-topic-list.hbs | 3 ++ .../stylesheets/common/base/_topic-list.scss | 12 +++-- app/models/topic_tracking_state.rb | 14 +++--- app/serializers/listable_topic_serializer.rb | 8 ++-- config/locales/client.en.yml | 1 + lib/svg_sprite/svg_sprite.rb | 1 + spec/models/topic_tracking_state_spec.rb | 17 +++++-- vendor/assets/svg-icons/fontawesome/solid.svg | 4 ++ 12 files changed, 107 insertions(+), 37 deletions(-) create mode 100644 app/assets/javascripts/discourse/templates/list/unread-indicator.raw.hbs diff --git a/app/assets/javascripts/discourse/components/basic-topic-list.js.es6 b/app/assets/javascripts/discourse/components/basic-topic-list.js.es6 index e08bf9d5892..e8af6acc94f 100644 --- a/app/assets/javascripts/discourse/components/basic-topic-list.js.es6 +++ b/app/assets/javascripts/discourse/components/basic-topic-list.js.es6 @@ -33,6 +33,51 @@ export default Ember.Component.extend({ } }, + didInsertElement() { + this._super(...arguments); + + this.topics.forEach(topic => { + const includeUnreadIndicator = + typeof topic.unread_by_group_member !== "undefined"; + + if (includeUnreadIndicator) { + const unreadIndicatorChannel = `/private-messages/unread-indicator/${topic.id}`; + this.messageBus.subscribe(unreadIndicatorChannel, data => { + const nodeClassList = document.querySelector( + `.indicator-topic-${data.topic_id}` + ).classList; + + if (data.show_indicator) { + nodeClassList.remove("read"); + } else { + nodeClassList.add("read"); + } + }); + } + }); + }, + + willDestroyElement() { + this._super(...arguments); + + this.topics.forEach(topic => { + const includeUnreadIndicator = + typeof topic.unread_by_group_member !== "undefined"; + + if (includeUnreadIndicator) { + const unreadIndicatorChannel = `/private-messages/unread-indicator/${topic.id}`; + this.messageBus.unsubscribe(unreadIndicatorChannel); + } + }); + }, + + @computed("topics") + showUnreadIndicator(topics) { + return topics.some( + topic => typeof topic.unread_by_group_member !== "undefined" + ); + }, + click(e) { // Mobile basic-topic-list doesn't use the `topic-list-item` view so // the event for the topic entrance is never wired up. diff --git a/app/assets/javascripts/discourse/components/topic-list-item.js.es6 b/app/assets/javascripts/discourse/components/topic-list-item.js.es6 index e7ed6186b34..1116d97e610 100644 --- a/app/assets/javascripts/discourse/components/topic-list-item.js.es6 +++ b/app/assets/javascripts/discourse/components/topic-list-item.js.es6 @@ -38,16 +38,16 @@ export const ListItemDefaults = { didInsertElement() { this._super(...arguments); - if (this.includeReadIndicator) { - this.messageBus.subscribe(this.readIndicatorChannel, data => { + if (this.includeUnreadIndicator) { + this.messageBus.subscribe(this.unreadIndicatorChannel, data => { const nodeClassList = document.querySelector( `.indicator-topic-${data.topic_id}` ).classList; if (data.show_indicator) { - nodeClassList.remove("unread"); + nodeClassList.remove("read"); } else { - nodeClassList.add("unread"); + nodeClassList.add("read"); } }); } @@ -56,24 +56,24 @@ export const ListItemDefaults = { willDestroyElement() { this._super(...arguments); - if (this.includeReadIndicator) { - this.messageBus.unsubscribe(this.readIndicatorChannel); + if (this.includeUnreadIndicator) { + this.messageBus.unsubscribe(this.unreadIndicatorChannel); } }, @computed("topic.id") - readIndicatorChannel(topicId) { - return `/private-messages/read-indicator/${topicId}`; + unreadIndicatorChannel(topicId) { + return `/private-messages/unread-indicator/${topicId}`; }, - @computed("topic.read_by_group_member") - unreadClass(readByGroupMember) { - return readByGroupMember ? "" : "unread"; + @computed("topic.unread_by_group_member") + unreadClass(unreadByGroupMember) { + return unreadByGroupMember ? "" : "read"; }, - @computed("topic.read_by_group_member") - includeReadIndicator(readByGroupMember) { - return typeof readByGroupMember !== "undefined"; + @computed("topic.unread_by_group_member") + includeUnreadIndicator(unreadByGroupMember) { + return typeof unreadByGroupMember !== "undefined"; }, @computed diff --git a/app/assets/javascripts/discourse/templates/list/topic-list-item.raw.hbs b/app/assets/javascripts/discourse/templates/list/topic-list-item.raw.hbs index f6d2ddfc382..db3ca305695 100644 --- a/app/assets/javascripts/discourse/templates/list/topic-list-item.raw.hbs +++ b/app/assets/javascripts/discourse/templates/list/topic-list-item.raw.hbs @@ -20,14 +20,10 @@ {{~topic-featured-link topic}} {{~/if}} {{~raw-plugin-outlet name="topic-list-after-title"}} + {{~raw "list/unread-indicator" includeUnreadIndicator=topic.unread_by_group_member topicId=topic.id ~}} {{~#if showTopicPostBadges}} {{~raw "topic-post-badges" unread=topic.unread newPosts=topic.displayNewPosts unseen=topic.unseen url=topic.lastUnreadUrl newDotText=newDotText}} {{~/if}} - {{~#if includeReadIndicator}} - - {{~d-icon "book-reader"}} - - {{~/if}}
{{#unless hideCategory}} diff --git a/app/assets/javascripts/discourse/templates/list/unread-indicator.raw.hbs b/app/assets/javascripts/discourse/templates/list/unread-indicator.raw.hbs new file mode 100644 index 00000000000..d951dd486bb --- /dev/null +++ b/app/assets/javascripts/discourse/templates/list/unread-indicator.raw.hbs @@ -0,0 +1,5 @@ +{{~#if includeUnreadIndicator~}} +   + {{~d-icon "asterisk"}} + +{{~/if}} diff --git a/app/assets/javascripts/discourse/templates/mobile/components/basic-topic-list.hbs b/app/assets/javascripts/discourse/templates/mobile/components/basic-topic-list.hbs index 2c5eb2f4d5c..a344ce85975 100644 --- a/app/assets/javascripts/discourse/templates/mobile/components/basic-topic-list.hbs +++ b/app/assets/javascripts/discourse/templates/mobile/components/basic-topic-list.hbs @@ -16,6 +16,9 @@
{{topic-status topic=t}} {{topic-link t}} + {{raw "list/unread-indicator" includeUnreadIndicator=showUnreadIndicator + topicId=t.id + unreadClass=(if t.unread_by_group_member "" "read")}} {{#if t.unseen}} {{/if}} diff --git a/app/assets/stylesheets/common/base/_topic-list.scss b/app/assets/stylesheets/common/base/_topic-list.scss index 0fd6593434d..59e8731d442 100644 --- a/app/assets/stylesheets/common/base/_topic-list.scss +++ b/app/assets/stylesheets/common/base/_topic-list.scss @@ -133,11 +133,15 @@ .raw-topic-link > * { pointer-events: none; } + } - .read-indicator { - &.unread { - display: none; - } + .unread-indicator { + &.read { + display: none; + } + .d-icon { + vertical-align: 0em; + font-size: $font-down-5; } } diff --git a/app/models/topic_tracking_state.rb b/app/models/topic_tracking_state.rb index 7f46f2c78c9..ed8f347bd1a 100644 --- a/app/models/topic_tracking_state.rb +++ b/app/models/topic_tracking_state.rb @@ -347,7 +347,7 @@ SQL if topic.private_message? groups = read_allowed_groups_of(topic) - update_topic_list_read_indicator(topic, groups, topic.highest_post_number, user_id, false) + update_topic_list_read_indicator(topic, groups, topic.highest_post_number, user_id, true) end end @@ -358,7 +358,7 @@ SQL groups = read_allowed_groups_of(topic) post = Post.find_by(topic_id: topic.id, post_number: last_read_post_number) trigger_post_read_count_update(post, groups) - update_topic_list_read_indicator(topic, groups, last_read_post_number, user_id, true) + update_topic_list_read_indicator(topic, groups, last_read_post_number, user_id, false) end end @@ -370,23 +370,23 @@ SQL .group('groups.id') end - def self.update_topic_list_read_indicator(topic, groups, last_read_post_number, user_id, read_event) + def self.update_topic_list_read_indicator(topic, groups, last_read_post_number, user_id, write_event) return unless last_read_post_number == topic.highest_post_number - message = { topic_id: topic.id, show_indicator: read_event }.as_json + message = { topic_id: topic.id, show_indicator: write_event }.as_json groups_to_update = [] groups.each do |group| member = group.members.include?(user_id) - member_writing = (!read_event && member) - non_member_reading = (read_event && !member) + member_writing = (write_event && member) + non_member_reading = (!write_event && !member) next if non_member_reading || member_writing groups_to_update << group end return if groups_to_update.empty? - MessageBus.publish("/private-messages/read-indicator/#{topic.id}", message, user_ids: groups_to_update.flat_map(&:members)) + MessageBus.publish("/private-messages/unread-indicator/#{topic.id}", message, user_ids: groups_to_update.flat_map(&:members)) end def self.trigger_post_read_count_update(post, groups) diff --git a/app/serializers/listable_topic_serializer.rb b/app/serializers/listable_topic_serializer.rb index be762bed44d..2b110290e87 100644 --- a/app/serializers/listable_topic_serializer.rb +++ b/app/serializers/listable_topic_serializer.rb @@ -26,7 +26,7 @@ class ListableTopicSerializer < BasicTopicSerializer :bookmarked, :liked, :unicode_title, - :read_by_group_member + :unread_by_group_member has_one :last_poster, serializer: BasicUserSerializer, embed: :objects @@ -122,15 +122,15 @@ class ListableTopicSerializer < BasicTopicSerializer PinnedCheck.unpinned?(object, object.user_data) end - def read_by_group_member + def unread_by_group_member # object#last_read_post_number is an attribute selected from a joined table. # See TopicQuery#append_read_state for more information. return false unless object.respond_to?(:last_read_post_number) - object.last_read_post_number >= object.highest_post_number + object.last_read_post_number < object.highest_post_number end - def include_read_by_group_member? + def include_unread_by_group_member? !!object.topic_list&.publish_read_state end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 94a3bae36de..c2dbb4c4d49 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1986,6 +1986,7 @@ en: group_request: "You need to request membership to the `{{name}}` group to see this topic" group_join: "You need join the `{{name}}` group to see this topic" group_request_sent: "Your group membership request has been sent. You will be informed when it's accepted." + unread_indicator: "No member has read the last post of this topic yet." # keys ending with _MF use message format, see https://meta.discourse.org/t/message-format-support-for-localization/7035 for details read_more_MF: "There { diff --git a/lib/svg_sprite/svg_sprite.rb b/lib/svg_sprite/svg_sprite.rb index 237cefd4179..413c0cedda2 100644 --- a/lib/svg_sprite/svg_sprite.rb +++ b/lib/svg_sprite/svg_sprite.rb @@ -21,6 +21,7 @@ module SvgSprite "arrows-alt-h", "arrows-alt-v", "at", + "asterisk", "backward", "ban", "bars", diff --git a/spec/models/topic_tracking_state_spec.rb b/spec/models/topic_tracking_state_spec.rb index 7a8c3c601eb..bb90a71a13e 100644 --- a/spec/models/topic_tracking_state_spec.rb +++ b/spec/models/topic_tracking_state_spec.rb @@ -256,7 +256,7 @@ describe TopicTrackingState do describe '#publish_read_private_message' do fab!(:group) { Fabricate(:group) } - let(:read_topic_key) { "/private-messages/read-indicator/#{@group_message.id}" } + let(:read_topic_key) { "/private-messages/unread-indicator/#{@group_message.id}" } let(:read_post_key) { "/topic/#{@group_message.id}" } let(:latest_post_number) { 3 } @@ -281,15 +281,26 @@ describe TopicTrackingState do context 'when the read indicator is enabled' do before { group.update!(publish_read_state: true) } - it 'does publish the read indicator' do + it 'publishes a message to hide the unread indicator' do message = MessageBus.track_publish(read_topic_key) do TopicTrackingState.publish_read_indicator_on_read(@group_message.id, latest_post_number, user.id) end.first expect(message.data['topic_id']).to eq @group_message.id + expect(message.data['show_indicator']).to eq false end - it 'does not publish the read indicator if the message is not the last one' do + it 'publishes a message to show the unread indicator when a non-member creates a new post' do + allowed_user = Fabricate(:topic_allowed_user, topic: @group_message) + message = MessageBus.track_publish(read_topic_key) do + TopicTrackingState.publish_read_indicator_on_write(@group_message.id, latest_post_number, allowed_user.id) + end.first + + expect(message.data['topic_id']).to eq @group_message.id + expect(message.data['show_indicator']).to eq true + end + + it 'does not publish the unread indicator if the message is not the last one' do not_last_post_number = latest_post_number - 1 Fabricate(:post, topic: @group_message, post_number: not_last_post_number) messages = MessageBus.track_publish(read_topic_key) do diff --git a/vendor/assets/svg-icons/fontawesome/solid.svg b/vendor/assets/svg-icons/fontawesome/solid.svg index 1a9ad774d9a..11d9cc09295 100644 --- a/vendor/assets/svg-icons/fontawesome/solid.svg +++ b/vendor/assets/svg-icons/fontawesome/solid.svg @@ -3748,4 +3748,8 @@ Book Reader + + Asterisk + +