Skip to content

1993 improve proposals admin #2419

Merged
mrcasals merged 15 commits intomasterfrom
1993_improve_proposals_admin
Jan 10, 2018
Merged

1993 improve proposals admin #2419
mrcasals merged 15 commits intomasterfrom
1993_improve_proposals_admin

Conversation

@isaacmg410
Copy link
Copy Markdown
Contributor

@isaacmg410 isaacmg410 commented Dec 22, 2017

🎩 What? Why?

📌 Related Issues

📋 Subtasks

  • Sort by ID
  • Sort by title
  • Sort by votes
  • Sort by status
  • Sort by creation date
  • Sort by number of comments

decidim-comments (= 0.9.0.pre)
decidim-core (= 0.9.0.pre)
kaminari (~> 1.0.1)
ransack
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ransack looks nice, maybe we should drop searchlight in favor of it? @deivid-rodriguez @josepjaume @mrcasals

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me, although I just gave it a superficial look. Should we open an issue to discuss this?

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 3, 2018

Codecov Report

Merging #2419 into master will decrease coverage by <.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #2419      +/-   ##
==========================================
- Coverage   98.69%   98.68%   -0.01%     
==========================================
  Files        1296     1296              
  Lines       30279    30285       +6     
==========================================
+ Hits        29883    29887       +4     
- Misses        396      398       +2

@josepjaume
Copy link
Copy Markdown
Contributor

Is this PR properly rebased to master? I'm seeing a lot of unrelated changes in GitHub's diff.

@isaacmg410 isaacmg410 force-pushed the 1993_improve_proposals_admin branch from fff5b43 to 7e6ecdd Compare January 8, 2018 08:39
@isaacmg410
Copy link
Copy Markdown
Contributor Author

@josepjaume I'm checking the rebase.

Does anyone of @decidim/lot-core can review the comments on this issue and give feedback on how to implement the counter_cache for comments?

@isaacmg410 isaacmg410 force-pushed the 1993_improve_proposals_admin branch from 3581efc to 0851e97 Compare January 9, 2018 08:07
@isaacmg410 isaacmg410 changed the title [WIP] 1993 improve proposals admin 1993 improve proposals admin Jan 9, 2018
proposal:
fields:
category: Category
comments: Comments
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be "Comments count"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've followed the same naming as "votes: Votes". If "Comments count" is prefered I can change

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I see. Let's leave it as it is then! 😄

@mrcasals
Copy link
Copy Markdown
Contributor

@isaacmg410 PR looks good, nice job! Can you add a CHANGELOG entry before merging this, please?

@isaacmg410
Copy link
Copy Markdown
Contributor Author

@mrcasals changelog entry added.

mrcasals
mrcasals previously approved these changes Jan 10, 2018
Copy link
Copy Markdown
Contributor

@oriolgual oriolgual left a comment

Choose a reason for hiding this comment

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

Just minor things, nice work!

helper_method :proposals
helper_method :proposals, :query

def index; end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rails doesn't need the action definition is there's already the view file, so we can delete this!

private

def query
@q ||= Proposal.where(feature: current_feature).ransack(params[:q])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use @query as a variable name

@mrcasals mrcasals merged commit eae6367 into master Jan 10, 2018
@mrcasals mrcasals deleted the 1993_improve_proposals_admin branch January 10, 2018 10:48
@ghost ghost removed the in-review label Jan 10, 2018
@mrcasals
Copy link
Copy Markdown
Contributor

Wohoo! 🎉 💪

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.

4 participants