Conversation
|
@beagleknight, thanks for your PR! By analyzing the history of the files in this pull request, we identified @josepjaume, @oriolgual and @mrcasals to be potential reviewers. |
There was a problem hiding this comment.
Missing top-level module documentation comment.
There was a problem hiding this comment.
Indent the first parameter one step more than options.merge!(.
There was a problem hiding this comment.
Missing top-level module documentation comment.
f823abe to
e80c261
Compare
Current coverage is 61.00% (diff: 100%)@@ master #250 diff @@
==========================================
Files 2976 3053 +77
Lines 122687 123694 +1007
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 74495 75456 +961
- Misses 48192 48238 +46
Partials 0 0
|
d07c5bc to
fd76c2c
Compare
There was a problem hiding this comment.
Missing top-level module documentation comment.
There was a problem hiding this comment.
Missing top-level module documentation comment.
There was a problem hiding this comment.
Indent the first parameter one step more than options.merge!(.
There was a problem hiding this comment.
Indent the first parameter one step more than options.merge!(.
There was a problem hiding this comment.
Missing top-level module documentation comment.
There was a problem hiding this comment.
Missing top-level module documentation comment.
67cc7fa to
30f9b07
Compare
| field :body, !types.String, "The comment message" | ||
| end | ||
| end | ||
| end No newline at end of file |
| end | ||
| end | ||
| end | ||
| end No newline at end of file |
| @@ -0,0 +1,12 @@ | |||
| module Decidim | |||
| module Comments | |||
| class Comment | |||
There was a problem hiding this comment.
Missing top-level class documentation comment.
| type !types[ CommentType] | ||
| description "Lists all comments." | ||
|
|
||
| resolve -> (_obj, _args, ctx) { |
There was a problem hiding this comment.
Unused block argument - ctx. If it's necessary, use _ or _ctx as an argument name to indicate that it won't be used. Also consider using a proc without arguments instead of a lambda if you want it to accept any arguments but don't care about them.
| end | ||
|
|
||
| field :comments do | ||
| type !types[ CommentType] |
There was a problem hiding this comment.
Space inside square brackets detected.
There was a problem hiding this comment.
Use the lambda method for multiline lambdas.
Unused block argument - ctx. If it's necessary, use _ or _ctx as an argument name to indicate that it won't be used.
There was a problem hiding this comment.
Use snake_case for variable names.
There was a problem hiding this comment.
Missing frozen string literal comment.
6b0ed7d to
8faf977
Compare
| let!(:participatory_process) { create(:participatory_process, organization: current_organization) } | ||
|
|
||
| let(:query) { | ||
| "{ addComment(commentableId: \"#{participatory_process.id}\", commentableType: \"Decidim::ParticipatoryProcess\", body: \"This is a new comment\") { id, body } }" |
There was a problem hiding this comment.
It feels weird to me that the frontend knows internals of the backend (ie the name of the class of the participatory process). The CommentsHelper sets automatically this field (resource.class.name), but it still feels weird to me that the frontend knows these things. If we want to make a mobile app that is not linked with the Rails backend, so that it can't set resource.class.name, then the mobile app needs to know the name of the Rails class.
There was a problem hiding this comment.
I'd rather have an addProcessComments(id, body), or an addComments scoped to a process(it that exists in GraphQL)
There was a problem hiding this comment.
I agree with you. We can tweak de API in a future PR. It's not a big deal because graphql and apollo make changes easy 😄
| let!(:participatory_process) { create(:participatory_process, organization: current_organization) } | ||
|
|
||
| let(:query) { | ||
| "{ addComment(commentableId: \"#{participatory_process.id}\", commentableType: \"Decidim::ParticipatoryProcess\", body: \"This is a new comment\") { id, body } }" |
There was a problem hiding this comment.
I'd rather have an addProcessComments(id, body), or an addComments scoped to a process(it that exists in GraphQL)
| attr_reader :form | ||
|
|
||
| def create_comment | ||
| @comment = Comment.create(author: @author, |
There was a problem hiding this comment.
We need to solve this before merging
| Gem::Specification.new do |s| | ||
| Decidim.add_default_gemspec_properties(s) | ||
|
|
||
| s.files = Dir["{app,config,db,lib,vendor}/**/*", "LICENSE.txt", "Rakefile", "README.md"] |
There was a problem hiding this comment.
The LICENSE.txt file does not exist.
|
|
||
| field :name, types.String, "The user's name" | ||
| end | ||
| end No newline at end of file |
| # | ||
| # Returns a div which contain a RectComponent to be rendered by `react_ujs` | ||
| def comments_for(resource) | ||
| react_component("Comments", commentableType: resource.class.name, commentableId: resource.id.to_s, locale: I18n.locale) + javascript_include_tag("decidim/comments/comments") |
| describe "currentUser" do | ||
| let(:query) { "{ currentUser { name } } " } | ||
|
|
||
| context "If the user is logged in" do |
| end | ||
| end | ||
|
|
||
| context "If the user is not logged in" do |
|
|
||
| helper.comments_for(commentable) | ||
| end | ||
| it "should render the react component `Comments` with the correct data" do |
| # This type represents a User. | ||
| UserType = GraphQL::ObjectType.define do | ||
| name "User" | ||
| description "An user" |
|
|
||
| field :currentUser do | ||
| type UserType | ||
| description "Return's information about the user logged in" |
There was a problem hiding this comment.
s/about the user logged in/about the logged in user
There was a problem hiding this comment.
Use snake_case for variable names.
There was a problem hiding this comment.
Use snake_case for variable names.
ecef8ed to
a6022db
Compare
🎩 What? Why?
This adds the engine
decidim-commentsand will add the capability to add comments to any component using a simple Rails helper like this:This is the first engine using frontend components with React. Since is an isolated engine it's using its own way to compile assets merging the Rails assets pipeline with Webpack.
We are gonna start simple: just one level of hierarchy without extra features like voting or arguing comments.
📋 Related documentation
📌 Related Issues
📋 Subtasks
Pagination(out of scope for this pr)Authentication(out of scope for this pr, fallback to cookies)📷 Screenshots (optional)
None
👻 GIF
None