Skip to content
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

Comments #17

Merged
merged 11 commits into from
Jun 18, 2024
Merged

Comments #17

merged 11 commits into from
Jun 18, 2024

Conversation

paulmckissock
Copy link
Contributor

Each article page lists all comments. Users can comment on each article page.

@paulmckissock paulmckissock requested a review from noahko96 June 17, 2024 19:40
Copy link
Contributor

@noahko96 noahko96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some thoughts about limiting the number of displayed comments

app/controllers/articles_controller.rb Outdated Show resolved Hide resolved
@paulmckissock paulmckissock requested a review from noahko96 June 17, 2024 20:42
Copy link
Contributor

@noahko96 noahko96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more things. Also, it does not look like you added any tests or seeds around comments. Should definitely do both of those things.

app/views/users/articles_written.html.erb Outdated Show resolved Hide resolved
app/views/users/articles_written.html.erb Outdated Show resolved Hide resolved
config/routes.rb Outdated Show resolved Hide resolved
app/views/users/articles_written.html.erb Outdated Show resolved Hide resolved
@paulmckissock paulmckissock requested a review from noahko96 June 18, 2024 14:51
Copy link
Contributor

@noahko96 noahko96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple other small things

@@ -0,0 +1,22 @@
class CommentsController < ApplicationController
before_action :authorized, only: [:create]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create is the only action in this controller, so the only is unnecessary.

config/routes.rb Outdated
@@ -1,6 +1,8 @@
Rails.application.routes.draw do
root "articles#index"
resources :articles
resources :articles do
resources :comments, only: [:create, :index]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments controller doesn't have an index action, so I would either remove this index route or add a corresponding action and view.

@paulmckissock paulmckissock requested a review from noahko96 June 18, 2024 15:22
Copy link
Contributor

@noahko96 noahko96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@paulmckissock paulmckissock merged commit 206fc52 into main Jun 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants