Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added js controls logic - control prepend or append for comments #15

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rosscdh
Copy link

@rosscdh rosscdh commented Jun 6, 2013

Added js controls logic which is backwards compatible but allows is_reversed to be specified

@vdboor
Copy link
Contributor

vdboor commented Jun 6, 2013

Hi! This is a neat feature to have, I like adding it.

There are a few things I'd prefer to see updated though:

  • Give a different name to COMMENT_CONTROLS. Perhaps something like FLUENT_COMMENTS_SETTINGS (or config) or even <div id="comments" data-comments-reversed="true">). COMMENT_CONTROLS is too vague imho.
  • Would it be possible to automatically include this setting value via the template tags, or is that information not available at that point?

Thanks for your time, and thanks in advance for looking at these points.

@rosscdh
Copy link
Author

rosscdh commented Jun 6, 2013

Hey there, sure no problem; thanks for the nice app that plugs neatly in :) 💯

  1. I considered implementing it using data-* and probably still should so that the user can override the actual template.
    1. for my use-case I didnt want to have to override the template, and rather just modify direction based on the JS var.
  2. I have already created (in my project) another template tag called *_reveresed which I am happy to contribute to fluent_comment. Using that it will then inject the is_reversed variable as a data-is_reversed and return the queryset in the reverse order

My pleasure :)
Ross Crawford-d'Heureuse

@@ -93,7 +98,7 @@

function scrollToElement( $element, speed, offset )
{
if( $element.length )
if( $element.length && COMMENT_CONTROLS.scroll_to_comment === true )
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why the explicit === true ?

Copy link
Author

Choose a reason for hiding this comment

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

I was working on the assumption that if the user enters "false" it will evaluate as true.
var test = {};
test.item = 'false'
"false"
if (test.item) { console.log('yay') }

@vdboor
Copy link
Contributor

vdboor commented Jun 17, 2013

Hi, I've made some additional comments in the diff. Let me know what you think of these! I hope you don't mind a critical look here.

@rosscdh
Copy link
Author

rosscdh commented Jun 17, 2013

Hi there Diederik, of course not :) critical comments are welcome! if were gonna release something we had better make sure its more than just "ok".

I'm a touch busy atm but will give this a closer look soon!

@euanlau
Copy link

euanlau commented Jul 9, 2013

Before adding more features, is it better to rewrite the thing to a proper jQuery plugin? Actually I filed an issue #23 just for that.
Right now we have to modify ajaxcomments.js itself in order to customize the behavior.
It would be much better to have something like this:

$('div.comments-container').fluentcomments({
    append: true,
    scrollToComment: false,
});

@vdboor
Copy link
Contributor

vdboor commented Jul 9, 2013

I totally agree on that one @euanlau! Nice idea. It would also make the new multi-form support (currently in master) easier to use.

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

Successfully merging this pull request may close these issues.

4 participants