Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upThread read notifications #417
Conversation
hmadison
changed the title from
Hm/thread read notifications
to
Thread read notifications
Oct 8, 2017
pushcx
requested changes
Oct 13, 2017
Thanks for taking a first pass at this, it'll help keep discussions going.
I think there's enough going on here that replies need their own controller, and it should present sub-navigation to pages listing unread replies, all replies, comment replies, and story replies. Additionally, it shouldn't be necessary to click 'mark read', they should be considered read if they've been rendered to page (which can be accomplished easiest by not paginating the unread page).
It also needs to be possible to unsubscribe from seeing replies to a story or comment. I will admit to having posted one or two stories on topics I'm not interested in reading the comments on.
pushcx
assigned
hmadison
Oct 13, 2017
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
I've made the changes you requested. The new replies page:
|
pushcx
requested changes
Jan 29, 2018
This looks really good. I'm going to play with it a bit locally to see the UI and such, but wanted to post the small style nits and questions so you can see them immediately. I'm looking forward to merging this.
app/controllers/comments_controller.rb
| @@ -4,7 +4,7 @@ class CommentsController < ApplicationController | ||
| # for rss feeds, load the user's tag filters if a token is passed | ||
| before_action :find_user_from_rss_token, :only => [ :index ] | ||
| before_action :require_logged_in_user_or_400, | ||
| :only => [ :create, :preview, :upvote, :downvote, :unvote ] | ||
| :only => [ :create, :preview, :upvote, :downvote, :unvote] |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| @@ -0,0 +1,3 @@ | ||
| class ApplicationRecord < ActiveRecord::Base |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
pushcx
Jan 29, 2018
Member
How about a name like ApplicationView to make it clear what this is for and why it's not used on other models?
pushcx
Jan 29, 2018
Member
How about a name like ApplicationView to make it clear what this is for and why it's not used on other models?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
hmadison
Jan 29, 2018
Contributor
See rails/rails#22567. New rails 5 projects generate this by default.
hmadison
Jan 29, 2018
Contributor
See rails/rails#22567. New rails 5 projects generate this by default.
app/models/read_ribbon.rb
| belongs_to :user | ||
| belongs_to :story | ||
| class << self |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
pushcx
Jan 29, 2018
Member
Codebase style is to do def self.hide_replies_for rather than the nested class definition and I'd like to stick to it.
pushcx
Jan 29, 2018
Member
Codebase style is to do def self.hide_replies_for rather than the nested class definition and I'd like to stick to it.
| @@ -8,6 +8,7 @@ class StoriesController < ApplicationController | ||
| before_action :find_user_story, :only => [ :destroy, :edit, :undelete, | ||
| :update ] | ||
| before_action :find_story!, :only => [ :suggest, :submit_suggestions ] | ||
| around_action :track_story_reads, only: [ :show ], if: -> { @user.present? } |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| @@ -0,0 +1,43 @@ | ||
| <div class="box wide"> | ||
| <div class="legend right"> | ||
| <%= link_to_filter('unread') %> | |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
app/views/replies/show.html.erb
| <%= render "comments/comment", | ||
| comment: reply.comment, | ||
| show_story: true, | ||
| is_unread: reply.is_unread %> |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
app/views/replies/show.html.erb
| <% end %> | ||
| </ol> | ||
| <% @filter != 'unread' && if @replies.count > RepliesController::REPLIES_PER_PAGE %> |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| @@ -56,7 +56,8 @@ | ||
| :show_story => (comment.story_id != @story.id), | ||
| :show_tree_lines => true, | ||
| :was_merged => (comment.story_id != @story.id), | ||
| :children => children %> | ||
| :children => children, | ||
| :is_unread => is_unread?(comment) %> |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| @@ -0,0 +1,3 @@ | ||
| Scenic.configure do |config| | ||
| config.database = Scenic::Adapters::Mysql.new |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
pushcx
Jan 29, 2018
Member
Can we avoid saying MySQL here and pull it from Rails.config or database.yml? I'm thinking of barnacles, the other sister sites using postgresql, and my daydream of moving Lobsters to pg. I saw that Gemfile gets scenic-mysql added; anyone using pg now is already forked there, just hoping to minimize their edit points.
pushcx
Jan 29, 2018
Member
Can we avoid saying MySQL here and pull it from Rails.config or database.yml? I'm thinking of barnacles, the other sister sites using postgresql, and my daydream of moving Lobsters to pg. I saw that Gemfile gets scenic-mysql added; anyone using pg now is already forked there, just hoping to minimize their edit points.
db/migrate/20171018192044_create_read_ribbons.rb
| class CreateReadRibbons < ActiveRecord::Migration[5.1] | ||
| def change | ||
| create_table :read_ribbons do |t| | ||
| # t.belongs_to :user, foreign_key: true |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
pushcx
Jan 29, 2018
Member
Why comment this out to do in a second migration? And can the create_view migration get folded in to this one as well?
pushcx
Jan 29, 2018
Member
Why comment this out to do in a second migration? And can the create_view migration get folded in to this one as well?
| comments parent_comments ON parent_comments.id = comments.parent_comment_id | ||
| WHERE | ||
| read_ribbons.is_following = 1 | ||
| AND comments.user_id != read_ribbons.user_id |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
pushcx
Jan 30, 2018
Member
I slept on this PR and it occurred to me: please add AND (comments.upvotes - comments.downvotes) < 0 AND (parent_comments.upvotes - parent_comments.downvotes) < 0 (we can memoize a score column if this becomes a performance issue). I want this reply tracking because it's going to help sustain really good conversations, I don't want it to sustain flame wars.
pushcx
Jan 30, 2018
Member
I slept on this PR and it occurred to me: please add AND (comments.upvotes - comments.downvotes) < 0 AND (parent_comments.upvotes - parent_comments.downvotes) < 0 (we can memoize a score column if this becomes a performance issue). I want this reply tracking because it's going to help sustain really good conversations, I don't want it to sustain flame wars.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
hmadison
added some commits
Oct 8, 2017
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
pushcx
Jan 31, 2018
Member
Thanks for these last tweaks and all the little style conformance nitpicking.
I just hit 'approve' on the review, but will hold on hitting merge and pushing until tomorrow morning so I can make an announce post as @talklittle suggested.
|
Thanks for these last tweaks and all the little style conformance nitpicking. I just hit 'approve' on the review, but will hold on hitting merge and pushing until tomorrow morning so I can make an |
| @ribbon = ReadRibbon.where(user_id: @user.id, story_id: story.id).first_or_create | ||
| yield | ||
| @ribbon.updated_at = Time.now | ||
| @ribbon.save |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
pushcx
Jan 31, 2018
Member
Good idiom to use, thanks. I don't know what @hmadison's schedule is this morning, so I'm going to merge and then commit that myself.
pushcx
Jan 31, 2018
Member
Good idiom to use, thanks. I don't know what @hmadison's schedule is this morning, so I'm going to merge and then commit that myself.
hmadison commentedOct 8, 2017
Resolves #218.
Adds a new page which shows all of the immediate child comments of stories/comments you have posted. This should help keep discussion on stories which fall off the front page from dying off.
Unread replies shown in navigation:
The page itself: