Simplify comments list behavior so API requests are consistent with other types of records. #647
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ther types of records.
|
@begedin sure, create an issue, but make it low priority. |
joshsmith
approved these changes
Oct 26, 2016
sbatson5
approved these changes
Oct 26, 2016
| setupController(controller, task) { | ||
| let user = this.get('currentUser.user'); | ||
| let newComment = this.store.createRecord('comment', { user }); | ||
| return controller.setProperties({ newComment, task }); |
There was a problem hiding this comment.
I think the return here isn't necessary. setupController does not return a value by default:
http://emberjs.com/api/classes/Ember.Route.html#method_setupController
It should be fine to just do controller.setProperties({ newComment, task }); without the return
| controller.set('newComment', models.comment); | ||
| controller.set('comments', models.comments); | ||
| setupController(controller, task) { | ||
| let user = this.get('currentUser.user'); |
There was a problem hiding this comment.
This is out of the scope of this PR, but I never like fetching properties directly from Services. It would be nice to have a getUser method:
this.get('currentUser').getUser();
There was a problem hiding this comment.
@sbatson5 so we don't lose this, would you want to open an issue here or do a quick PR for this?
…ther types of records.
joshsmith
force-pushed
the
644-simplify-comments
branch
from
7c6c7d2
to
819eb85
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's in this PR?
This makes it so that, on the
project.tasks.taskroute, instead of explicitly fetching comments for the task, we instead simply bind totask.commentsinstead.This change causes the following
projects/:id/tasks/:number/comments/route. Instead, we simply fetch from/comments/using coalesced id requests.modelandsetupControllerhooks for theproject.tasks.taskroute are much simpler.Further options
We could potentially further simplify things.
The
setupControllerhook could check if the session is authenticated and simply push a new comment into thetask.commentscollection.The
comment-list-componentcould then be the one that displays either acomment-itemcomponent, or some sort ofcomment-item-editcomponent, which can be used both for new and existing comments, depending on anisEditingflag.I believe it would reduce and dry up the code related to this section of the app a good deal, but it can be done in a separate PR. @joshsmith I can create an issue if you think it makes sense to do so. The behavior would be similar to what we're doing with donation goals.
References
Fixes #644
Progress on: code-corps/code-corps-api#372