-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adding ability to follow and unfollow users #119
Conversation
# Conflicts: # app/helpers/submissions_helper.rb # app/views/submissions/index.html.slim
…rtist-standing into feature/followers
# Conflicts: # app/views/submissions/index.html.slim
…rtist-standing into feature/followers
# Conflicts: # app/assets/stylesheets/users.scss # app/controllers/application_controller.rb # app/controllers/submissions_controller.rb # app/controllers/users_controller.rb # app/helpers/application_helper.rb # app/helpers/submissions_helper.rb # app/views/submissions/show.html.slim # app/views/users/_editform.html.slim # app/views/users/show.html.slim # config/routes.rb
def follow | ||
if Follower.where({ follower_user_id: current_user&.id, followed_user_id: params[:id] }).empty? | ||
Follower.create(follower_user_id: current_user&.id, followed_user_id: params[:id]) | ||
end | ||
|
||
redirect_back(fallback_location: root_path) | ||
end | ||
|
||
def unfollow | ||
Follower.where({ follower_user_id: current_user&.id, followed_user_id: params[:id] }) | ||
&.first | ||
&.destroy | ||
redirect_back(fallback_location: root_path) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make more sense for this to live in a followers_controller.rb, instead of the generic base application_controller?
config/routes.rb
Outdated
get 'follow/:id/' => 'application#follow' | ||
get 'unfollow/:id/' => 'application#unfollow' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see previous comment about creating a controller for follow actions
t.references :follower_user | ||
t.references :followed_user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems a little clumsy name-wise
the table and model is referred to as "Follower"
but what it really represents is a map between a user, and the user it's following
in my opinion, the model should probably be one of: "Following", "FollowMap".
the following_user should simply be user (or user_id)
the followed_user should simply be followed (or followed_id)
i.e. a "Following" or "FollowMap" represents an instance of a user following the followed,
god 'follow' stopped looking like a word to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thought of an alternative:
a "Follower" represents a user, and the following
user: user being followed
following: user following the user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm confused by your second comment. i just changed it to user_id and followed_id
span.fa.fa-edit | ||
| Edit Profile | ||
br | ||
= render partial: "application/follower_button", locals: {user: @user} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smart
| Unfollow | ||
- else | ||
a.btn.btn-primary href="/follow/#{user.id}" onclick="javascript:follow(#{user.id}); return false;" id="follow--#{user.id}" | ||
| Follow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I neglected the notion of partials when I started as a web dev and I'm definitely still paying for it.
Thanks for doing a significant part in lowering the future technical debt.
app/models/follower.rb
Outdated
class Follower < ApplicationRecord | ||
belongs_to :follower_user, :class_name => 'User' | ||
belongs_to :followed_user, :class_name => 'User' | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments on the migration
app/views/users/_editform.html.slim
Outdated
= image_tag f.followed_user.profile_picture | ||
a href="/users/#{f.followed_user.id}" | ||
p #{f.followed_user.username} | ||
= render partial: "application/follower_button", locals: {user: f.followed_user} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goddamn thats clean and smort
@@ -38,19 +38,20 @@ javascript: | |||
h2 = "#{@date == Time.now.utc.to_date ? "Today's" : @date.strftime("%B %e, %Y")} Submissions" | |||
|
|||
= form_tag submissions_path, method: :get do | |||
= date_field_tag :date, @date, class: "form-control", style: "width: 150px; display: inline-block; vertical-align: middle;" | |||
= date_field_tag :date, @date, class: "form-control", style: "width: 150px; display: inline-block; vertical-align: middle;", max: "#{Time.now.utc.to_date}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice small fix
a.btn.btn-primary style="margin-left: 5px; width: 100px; vertical-align: top;" href="#{submissions_path(:date => @date + 1.day)}" | ||
| Next Day | ||
- unless @date == Time.now.utc.to_date | ||
a.btn.btn-primary style="margin-left: 5px; width: 100px; vertical-align: top;" href="#{submissions_path(:date => @date + 1.day)}" disabled="#{}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does disabled="#{}" do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably just forgot to remove it.
app/helpers/submissions_helper.rb
Outdated
base_submissions(only_safe = true).where('submissions.nsfw_level = 1 and submissions.created_at >= ?', Time.now.utc - 14.days).sample | ||
end | ||
end | ||
# frozen_string_literal: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not in a place where i can easily compile things (haven't gotten around to setting up WSL2 in the hotel), can you give me a screenshot of how followed submissions bubbling to the top appear?
app/helpers/application_helper.rb
Outdated
include SessionsHelper | ||
|
||
def follows?(follower_id) | ||
return false unless logged_in? | ||
|
||
Follower.where({ follower_user_id: current_user&.id, followed_user_id: follower_id }).any? | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since application_helper is meant to be a lot of top-level stuff, consider making a followers_helper.rb and moving it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean, clean, clean. Few comments about a few things, but ultimately, this should be good to go pretty quickly.
# Conflicts: # app/controllers/application_controller.rb # app/controllers/submissions_controller.rb # app/controllers/users_controller.rb # app/helpers/application_helper.rb # app/helpers/submissions_helper.rb # config/routes.rb
Functionality to Follow A Dad's Submission