Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

comment threading proof of concept #74

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

See #28.

Owner

wilr commented Sep 23, 2013

Thanks for the proof, looks good. Interested to have comments on a comment recursion idea rather than a hierarchy model. Not sure how I feel on that. I was assuming a more implicit Parent/Child

I agree this should be opt in, so good idea on the config flag.

For this to be accepted as a feature I want to see unit tests on the backend functionality. Some cases off the the top of my head that the tests will need to support.

  • If a user cannot view a comment, they shouldn't be able to reply to it.
  • What happens if you delete a comment with children? I would lead towards simply replacing the content with a message (i.e "This message has been deleted") rather than deleting the whole tree which would make the conversation disjointed. (any behaviour should be at the API level - canDelete, onAfterDelete etc) so that this works across the board.

anselmdk commented Oct 4, 2013

I think the more implicit parent/child would make sense, as this should be a built-in feature. I see there is more in it than making this proof of concept if you want it into the core. I think this should be a good start for anybody who wants this feature now, but getting it into core should take another thought. I'll try to convince some clients that this is cool, so I can have some more time working on it :)

Contributor

tractorcow commented Feb 12, 2015

I see a problem with this approach if you wanted to gracefully degrade in environments where javascript is disabled. At the moment the current comments module should work without it, but additional scripts allow functionality to be enhanced in case it is enabled. :)

@dhensby dhensby commented on the diff Feb 13, 2015

code/controllers/CommentingController.php
@@ -207,6 +208,36 @@ public function approve() {
}
/**
+ * Renders html for replying to threaded comments
+ * Should be called via AJAX
+ * Example of url: /CommentingController/replyform/5?ReturnURL=/blog/sample-blog-entry/
+ *
+ * @return string
+ */
+ public function replyform(){
+ $p = $this->getURLParams();
+ $cID = (int) $p['ID'];
+ $c = Comment::get()->ByID($cID);
+
+ $controller = new CommentingController();
+ $controller->setOwnerRecord($c);
@dhensby

dhensby Feb 13, 2015

Owner

The controller needs the init() function running

@dhensby dhensby commented on the diff Feb 13, 2015

code/controllers/CommentingController.php
+ * @return string
+ */
+ public function replyform(){
+ $p = $this->getURLParams();
+ $cID = (int) $p['ID'];
+ $c = Comment::get()->ByID($cID);
+
+ $controller = new CommentingController();
+ $controller->setOwnerRecord($c);
+ $controller->setBaseClass('Comment');
+ $controller->setOwnerController(Controller::curr());
+
+ $form = $controller->CommentsForm();
+
+ //setting return url
+ if (isset($_GET['ReturnURL'])) {
@dhensby

dhensby Feb 13, 2015

Owner

Don't manipulate superglobals. use $this->request->getVar('ReturnURL');

@dhensby dhensby commented on the diff Feb 13, 2015

code/controllers/CommentingController.php
+
+ $controller = new CommentingController();
+ $controller->setOwnerRecord($c);
+ $controller->setBaseClass('Comment');
+ $controller->setOwnerController(Controller::curr());
+
+ $form = $controller->CommentsForm();
+
+ //setting return url
+ if (isset($_GET['ReturnURL'])) {
+ $form->loadDataFrom(array(
+ 'ReturnURL' => $_GET['ReturnURL'],
+ ));
+ }
+
+ return $form->forTemplate();
@dhensby

dhensby Feb 13, 2015

Owner

You don't need to explicitly render the form. If you don't then it makes extending the form easier.

Owner

dhensby commented Feb 13, 2015

I think this could fairly easily be made to work without JS too

Contributor

tractorcow commented Apr 22, 2015

I'm going to close this for now, since we've added nested comments in the master branch.

Thanks @anselmdk for your hard work and suggestions. :)

@tractorcow tractorcow closed this Apr 22, 2015

@tractorcow np - happy it's finally been implemented!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment