-
Notifications
You must be signed in to change notification settings - Fork 438
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
Add sorting option for comments #1005
Conversation
63d893b
to
ec0283b
Compare
ec0283b
to
d2ed180
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, you're being very diligent while trying to keep the diff to a minimum and not break existing things.
A few comments inline, mostly related to a (server-side) default for sort
and JS readability.
As for the after
parameter, with the switch to offset
it isn't needed by the frontend anymore. Did you keep it for backward compatibility? I could imagine a website admin only wanting to display recent comments (after = today-7days
) but they'd have to monkey-patch or recompile the embed JS to do that.
I know this is much to ask, but could you please add Jest
JS tests as well? Successively loading hidden comments is not tested at all at the moment (most of the still-few tests were written in a mad frenzy by me while trying to learn JS and Jest, so this is coming to bite us back).
Thank you for your feedback.
Yes, that's correct:
Sure, I will try to add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for incorporating the feedback.
The only thing I've noticed is that they can be unstable when running tests: they fail with a timeout error in different tests.
I've pushed a PR that should hopefully address this: #1021
This is good to go, if you could rebase and resolve the merge conflicts I would prefer to merge this before #993 (since both PRs touch some of the same files) Edit: (Jest tests can be added in another PR) |
a8e40dc
to
cf4cedd
Compare
Add sorting and pagination support for comments, including new API query parameters for sort order and offset. Adjust comment fetching logic accordingly. Closes isso-comments#4
cf4cedd
to
00fc383
Compare
@ix5 |
Tested locally as well with the demo page and I couldn't find any obvious flaws. Merged, thanks again for applying the feedback. For local testing/validating, I found this easy to quickly manipulate demo data with votes: diff --git a/isso/db/__init__.py b/isso/db/__init__.py
index e7209fe..d1ebb52 100644
--- a/isso/db/__init__.py
+++ b/isso/db/__init__.py
@@ -57,6 +57,7 @@ class SQLite3:
sql = ' '.join(sql)
with sqlite3.connect(self.path) as con:
+ con.set_trace_callback(logger.info)
return con.execute(sql, args)
@property
diff --git a/isso/demo/index.html b/isso/demo/index.html
index 1577253..61c804c 100644
--- a/isso/demo/index.html
+++ b/isso/demo/index.html
@@ -10,6 +10,10 @@
<h2><a href=".">Isso Demo</a></h2>
<script src="../js/embed.dev.js"
+ data-isso-sorting="upvotes" # or "newest" ...
+ data-isso-max-comments-top="1"
+ data-isso-max-comments-nested="2"
+ data-isso-reveal-on-click="2"
data-isso="../"
></script>
<!-- Uncomment to only test count functionality: -->
diff --git a/isso/views/comments.py b/isso/views/comments.py
index 9352dd0..933720d 100644
--- a/isso/views/comments.py
+++ b/isso/views/comments.py
@@ -409,6 +409,10 @@ class API(object):
Recipe source: https://stackoverflow.com/a/22936947/636849
"""
+
+ import random
+ return ".".join(4 * (str(random.randint(0, 255)),))
+
remote_addr = request.remote_addr
if self.trusted_proxies:
route = request.access_route + [remote_addr] |
@ix5 Do you plan to release this? |
Add sorting and pagination support for comments, including new API query parameters for sort order and offset. Adjust comment fetching logic accordingly.
Checklist
CHANGES.rst
because this is a user-facing change or an important bugfixWhat changes does this Pull Request introduce?
sort
) and pagination (offset
).data-isso-sorting
option.Why is this necessary?
Closes #4