search

ansonphong / postworld

Wordpress Theme Development Framework
GNU General Public License v2.0
7 stars 0 forks source link

load-comments Directive #41

Closed ansonphong closed 10 years ago

ansonphong commented 10 years ago

Hi Michel, I have added a directive called load-comments which basically does what the name says. It loads comments. Here is the documentation for that.

load-comments [ directive ] https://github.com/phongmedia/postworld/tree/master/js#load-comments--directive-

Two things I have done to get the process going:

  1. The main depednency function is complete: pw_get_comments() : https://github.com/phongmedia/postworld/tree/master/php#pw_get_comments--query-fields-tree-
  2. I have put in some template structure in these files:
    • templates/comments/comments-header.html
    • templates/comments/comment-single.html

Mind you, whatever is in the templates is very rough right now, and it is structurally incomplete, although it's a starting point which you can work from - please feel free to re-arrange the templates in any way you see fit.

The comments are meant to work in the same way as the comments on Reddit.

Let me know if you have any questions, and we can talk on Skype this week if you like to clarify any issues.

image

michelhabib commented 10 years ago

Phong, is the tree parameter in pw_get_comments working? i am only getting flat set of comments currently without children.

ansonphong commented 10 years ago

Yes it is currently working. I will find a good example of a post with hierarchical comments and an input / output for that...

ansonphong commented 10 years ago

Try post_id : 166220 -- this has lots of comments and several which are hierarchical. I'm testing locally now.

ansonphong commented 10 years ago

Perhaps if you try this:

echo json_encode(pw_get_comments(array("post_id"=>166220)));

You will see that some posts contain an object called 'children' which contains an array of child posts. This goes on recursively to however many levels deep the comments go.

michelhabib commented 10 years ago

i was trying post 40519, which i know it has many comments, but i only get the first level. i tried 166220, same thing. i am trying to trace it in php, it gets way too many comments, but when the tree is created, it seem to return first level only. i already set the tree parameter to true. i will give it more troubleshooting, maybe i can find something..

michelhabib commented 10 years ago

this is my code

    $query = array(
            'post_id' => 166220,            
        );
    $results = pw_get_comments ($query,'all',true);
michelhabib commented 10 years ago

i also noticed something strange, and i asked Ashraf to look into it, that comment_parent is usually a small number, that doesnt seem like a real comment_id, and mostly it is a number between 1 and 10, which would make it almost impossible to find a parent for any of these 5 digit comment_ids. would it be a script thing? maybe i should get a fresh copy of the migrated database from you?

ansonphong commented 10 years ago

Oh yes I know this issue - we fixed it a while ago. You need to run the most recent migration script on it. We fixed this at least a week ago, whereby the comment parent IDs weren't migrating, although Ashraf has already fixed this. The most recent migration script needs to be run.

michelhabib commented 10 years ago

confirmed and fixed. one more thing, user ids in wp_comments are not migrated, since they do not have associated roles. I discovered these as they were throwing warnings when getting comments. i will log that separately, but this is just a heads up, i guess it would need to revisit our migration script logic. this is not stopping me anyway, low priority for my work, but not sure of its priority in the db migration.

ansonphong commented 10 years ago

The user_id field in my wp_comments table has been migrated. When you ran the most recent migration script, did it not migrate the user_id?

On Wed, Oct 23, 2013 at 11:31 AM, michelhabib notifications@github.comwrote:

confirmed and fixed. one more thing, user ids in wp_comments are not migrated, since they do not have associated roles. I discovered these as they were throwing warnings when getting comments. i will log that separately, but this is just a heads up, i guess it would need to revisit our migration script logic. this is not stopping me anyway, low priority for my work, but not sure of its priority in the db migration.

— Reply to this email directly or view it on GitHubhttps://github.com/phongmedia/postworld/issues/41#issuecomment-26931510 .

michelhabib commented 10 years ago

I guess i described it wrong. Yes, The user ids are migrated in wp comments and yes, i ran the most recent script. The problem is that there are user ids in wp comments that do not have records in wp users, and the reason is that they do not have any roles associated to them. I will send you an example when i am on a computer.

Best Regards, Michel.

On Oct 23, 2013, at 11:51 PM, phong notifications@github.com wrote:

The user_id field in my wp_comments table has been migrated. When you ran the most recent migration script, did it not migrate the user_id?

On Wed, Oct 23, 2013 at 11:31 AM, michelhabib notifications@github.comwrote:

confirmed and fixed. one more thing, user ids in wp_comments are not migrated, since they do not have associated roles. I discovered these as they were throwing warnings when getting comments. i will log that separately, but this is just a heads up, i guess it would need to revisit our migration script logic. this is not stopping me anyway, low priority for my work, but not sure of its priority in the db migration.

— Reply to this email directly or view it on GitHubhttps://github.com/phongmedia/postworld/issues/41#issuecomment-26931510 .

— Reply to this email directly or view it on GitHub.

ansonphong commented 10 years ago

ok thanks...

On Wed, Oct 23, 2013 at 3:15 PM, michelhabib notifications@github.comwrote:

I guess i described it wrong. Yes, The user ids are migrated in wp comments and yes, i ran the most recent script. The problem is that there are user ids in wp comments that do not have records in wp users, and the reason is that they do not have any roles associated to them. I will send you an example when i am on a computer.

Best Regards, Michel.

On Oct 23, 2013, at 11:51 PM, phong notifications@github.com wrote:

The user_id field in my wp_comments table has been migrated. When you ran the most recent migration script, did it not migrate the user_id?

On Wed, Oct 23, 2013 at 11:31 AM, michelhabib notifications@github.comwrote:

confirmed and fixed. one more thing, user ids in wp_comments are not migrated, since they do not have associated roles. I discovered these as they were throwing warnings when getting comments. i will log that separately, but this is just a heads up, i guess it would need to revisit our migration script logic. this is not stopping me anyway, low priority for my work, but not sure of its priority in the db migration.

— Reply to this email directly or view it on GitHub< https://github.com/phongmedia/postworld/issues/41#issuecomment-26931510> .

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/phongmedia/postworld/issues/41#issuecomment-26950922 .

michelhabib commented 10 years ago

user id 21314 appears in wp comments, however it doesn't appear in wp users. if you check back in drupal, user 21314 exists, however it is not tied to any role, hence not migrated. Can you please check this one and let us know if that scenario is acceptable? no rush from my side anyways.

ansonphong commented 10 years ago

Interesting, I will look into that. That's a very good observation, I'm glad you noticed that. I will look into that as far as data migration is concerned.

On Wed, Oct 23, 2013 at 4:34 PM, michelhabib notifications@github.comwrote:

user id 21314 appears in wp comments, however it doesn't appear in wp users. if you check back in drupal, user 21314 exists, however it is not tied to any role, hence not migrated. Can you please check this one and let us know if that scenario is acceptable? no rush from my side anyways.

— Reply to this email directly or view it on GitHubhttps://github.com/phongmedia/postworld/issues/41#issuecomment-26955445 .

michelhabib commented 10 years ago

Phong, if i should use edit-field with comment editing and sorting, is it ready for such use? and would you recommend using it in this case, or use regular fields for this special case?

ansonphong commented 10 years ago

Hi Michel I have archived and disabled the edit-field directive, as I found the built-in Angular directives and controllers to be sufficient. You can use ng-options for dropdown select items, and regular fields with ng-model.

On Thursday, October 24, 2013, michelhabib wrote:

Phong, if i should use edit-field with comment editing and sorting, is it ready for such use? and would you recommend using it in this case, or use regular fields for this special case?

— Reply to this email directly or view it on GitHubhttps://github.com/phongmedia/postworld/issues/41#issuecomment-26995462 .

michelhabib commented 10 years ago

oh that's ok, i am about to finish and publish most of the load-comments component.

michelhabib commented 10 years ago

the wordpress comments do not have the option to flag a comment by a regular user, as far as i understand. i see the flagging option in your mockup above, is that something in scope?

ansonphong commented 10 years ago

you're right, comment flagging isn't in scope. Although the ability to delete a comment should be.

On Thu, Oct 24, 2013 at 2:53 PM, michelhabib notifications@github.comwrote:

the wordpress comments do not have the option to flag a comment by a regular user, as far as i understand. i see the flagging option in your mockup above, is that something in scope?

— Reply to this email directly or view it on GitHubhttps://github.com/phongmedia/postworld/issues/41#issuecomment-27034543 .

michelhabib commented 10 years ago

Phong, now load-comments directive is ready and committed into github. Here are my comments inline in the js file, showing what is completed and the leftovers/questions that i am finalizing now. i am expecting that you will work on the UI, but at the same time i made an effort to make it look as clean as possible - there is still some ui work to be done that i listed below.

/*
 * Tasks
 * Get Comments Service [OK]
 * Create Recursive Comment Structure [OK]
 *  - with Maximimze/minimize [OK] 
 *  - +/- Karma Points [OK - just add ajax Functions]
 * Create Comment Tempalte [OK]
 * Add Comment/Reply [OK]
 * Edit Comment [OK]
 * Delete Comment [OK]
 * Toggle Reply/Delete/Edit [OK]
 * Flag comment [Doesn't exist]
 * Remove ngAnimate to enhance performance! [OK]
 * Bind HTML/Sanitize Content/Format Content [OK]
 * NOTE: Karma points are always zero, even if updated in the wp_comments table, it seems they need to be updated somewhere else
 *  - Maximize/Minimize based on Points [OK] = Note that we are currently using a random function to generate points - for testing purposes
 * performance Tuning of initial loading time [OK] [using chrome timeline - less than 1 second to render - after loading data from server]
 * Create a Comment on the Top Level [OK]
 * OrderBy is moved to query field and underscore is removed, just to match the arguments format of the get_comments function
 * Sort Options and refresh based on sorting [OK]
 * Show as Tree or linear [OK]
 * Show Loading Icon while loading data [OK]
 * Permissions, you can only edit and delete your own comments [OK]
 * 
 *  - show control bar on hover only
 *  - show icons for control bar
 *  - highlight selected function of the control bar [reply, edit, delete, etc...]
 *  
 *  - Show More [in progress]
 * add Timezone to date
 * encapsulate in a simple directive
 * 
 * when adding new comments they should be open by default?
 * Can we use the html returned directly?

 * Cleanup UI
 * Refactor a bit
 * Cleanup CSS
 * Animation? Performance Limitations
 * If we can show the comments with tree=false, what happens when users reply? it will become a tree? no?
*/
michelhabib commented 10 years ago

Here are some screenshots, illustrating the different functions, loading, viewing nested comments, edit comment, delete comment, reply to comment

loading nestedcomments editcomment delete reply

ansonphong commented 10 years ago

Great, that looks good. I'll have a look at it today.

On Sat, Oct 26, 2013 at 6:37 AM, michelhabib notifications@github.comwrote:

Here are some screenshots, illustrating the different functions, loading, viewing nested comments, edit comment, delete comment, reply to comment

[image: loading]https://f.cloud.github.com/assets/296637/1413638/9fb33462-3e43-11e3-9625-b03029e3d4e8.jpg [image: nestedcomments]https://f.cloud.github.com/assets/296637/1413637/9fae93f8-3e43-11e3-9113-605f9931e964.jpg [image: editcomment]https://f.cloud.github.com/assets/296637/1413634/9ee8d55a-3e43-11e3-983f-ae911b6a2446.jpg [image: delete]https://f.cloud.github.com/assets/296637/1413635/9f683098-3e43-11e3-8577-ff25b5612dbb.jpg [image: reply]https://f.cloud.github.com/assets/296637/1413636/9fa63cf8-3e43-11e3-9a40-04323f187331.jpg

— Reply to this email directly or view it on GitHubhttps://github.com/phongmedia/postworld/issues/41#issuecomment-27146386 .

michelhabib commented 10 years ago

thanks, i made a bit of refactoring and cleaning, and hiding part of long comments [show more], the css is a bit messy. just let me know where to stop. i am just investigating a small issue related to the comment date and user timezone, they are not quire aligned.

ansonphong commented 10 years ago

ok good idea --- I have found working with dates, to stick with reading and writing _date_gmt, and then convert to the user display, and the back to GMT/UTC when writing to the DB. This way it doesn't matter if they've set their Wordpress timezone settings, it will use the offset of their current computer.

This is helpful: http://docs.angularjs.org/api/ng.filter:date

michelhabib commented 10 years ago

i am using the same way that wordpress is writing it to the database and that is exactly as you suggested, thanks for the tip. my problem was related to the AngularJS interpretation, it does not use the standard date format, which adds a Z to indicate that the date needs to be converted to local time. so , i just added the Z and it works now. the reason i could reproduce the issue is that i am using a server with a different timezone settings that the client. as for the date filters, i am using a custom filter called TimeAgo, which gives the relative time, and it looks good so far.

Thanks

ansonphong commented 10 years ago

Yes understood - the date often ends up taking longer than I expect, due to the various object formatting and time zone conversion.

ansonphong commented 10 years ago

By first testing, the load-comments system appears to be working very effectively. It's by far the most sophisticated rendering of comments I've seen done for Wordpress before. Good thing the system is open source.

I have a question - you may tell me if it's very easy to implement or outside of our current budget scope. What would it take to make a polling function with the following parameters?

load-comments [ directive ]

  live_poll: 60,  // in seconds
  live_poll_while_writing: false,  // boolean

live_poll : integer

live_poll_while_writing : boolean

Usage

load_comments['post_single'] = {

   // LIVE POLL PARAMETERS //
   live_poll: 60,
   live_poll_while_writing: false,

    query : {
        post_id : 133925, // 21853, // 133925, // 166220, // 21853, // 166220,
        status: 'approve',
        orderby : 'comment_date',
        },
    fields : 'all',
    tree : true,
    order_options : {
        'comment_points' : 'Points',
        'comment_date' : 'Date'
        },
    min_points : 0,
};
michelhabib commented 10 years ago

I can give it a try. The way it is designed, it might make this implementation fairly simple. The way i understand it is that every x seconds, the client will ping the server with the same get_comments function, retrieve all the results again, extract the difference and update its tree structure. unless you have a magical function that will return to the client all the adds, edits and deletes during the past period.

Theoritically, to do this, i need to:-

about live poll while writing, do you mean preventing the poll when the user is writing a comment?

michelhabib commented 10 years ago

and Thanks, it is all your idea, i just hope my implementation is close enough :)

michelhabib commented 10 years ago

Just a note here, AngularJS gets slower as we add new comments. I ran my test with the largest amount of comments available on the old site, that is 250 comments in a tree with 150 comments on the first level. and i guess the loading time is not that bad. However, if you do thousands of comments, this approach might not be the best. it would be crazy to have thousands of dynamic elements, because of all the checks that Angular needs to run.

Let me know if that would be a concern, as it is the only limitation i can find so far with our design.

ansonphong commented 10 years ago

I can understand about the possible performance issues with polling. The approach you suggested is good, by walking the comments tree twice, once to remove and once to add new comments, then re-render seamlessly.

You can give that a test and see how it goes. If it becomes overly complex or very slow on performance, then we can remove it from current scope.

michelhabib commented 10 years ago

Ok then. If i understand correctly, i will just give it lower priority than the rest of the remaining directives, i think we're very close - at least from my perspective here.

ansonphong commented 10 years ago

Yes, that polling can be lower priority. As you said that, I just noticed that GitHub did a polling an updated our comments in real-time, which was interesting. I was also writing a post while that happened and my post writing was not interrupted, so perhaps there would be no need to prevent polling while writing a comment.

I have 2 other questions which you may be able to help with.

Question 1

I tried a number of things, and wasn't able to get it to work - it seems like it would be straightforward.

So I'm using load-comments inside another template, where I have the ID of the post in scope as post.ID.

So I want to load the directive into this template, referencing the post.ID like this:

load_comments['post_single'] = {
    query : {
        post_id : post.ID, // This doesn't seem to work. Any ideas?
        status: 'approve',
     orderby : 'comment_date',
        },
    min_points : 20,
};

Question 2

The parameter min_points : 20 doesn't seem to be working. I'd expect it would only have posts 'expanded' which have minimum of 20 karma points. This could also help improve performance and usability, since the user would have to 'dig' if they want to keep reading down a thread, so we would only have to render the header of the comment, and not the comment body.

Was this min_points parameter implemented yet, or am I missing something?

I'm generally quite happy with how load-comments is working. Bravo.

michelhabib commented 10 years ago

Thanks Phong, I will get back to polling comments when i start with my trial, the test will help reveal more about the usability.

for Question 1, let me help troubleshoot it, if you replace post.ID with a number does it work? if no, then it is something with the template, it should be compared against the running environment in postworld-theme. if yes, then when exactly do you set post.ID value? is it before load_comments['post_single'] is defined or after? if before, then try to console.log('check',load_comments['post_single'].query.post_id) if after, this is not possible, the array must be fully defined before calling the directive. At least we have to think of how this should be addressed, not a big deal though.

for Question 2, Re-reading this, i guess i might have misinterpreted the requirement. The way i implemented it, is, 

    child.karmaPoints = child.comment_karma = Math.floor(Math.random() * 100) + 1;

i was thinking that you want the user to have all comments but have important ones only visible, the invisible ones can be seen if the selects to do so.

now, i guess you mean that you need

Sorry i am getting into tiny details, but that will help me better understand the requirements. and sorry if there is a misunderstanding here, i am sure i can cover it up quickly with the current design.

ansonphong commented 10 years ago

Question 1 : Loading post.ID into load_comments[]