Add sorting option for comments#1005
Conversation
63d893b to
ec0283b
Compare
ec0283b to
d2ed180
Compare
ix5
left a comment
There was a problem hiding this comment.
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 |
|
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.rstbecause this is a user-facing change or an important bugfixWhat changes does this Pull Request introduce?
sort) and pagination (offset).data-isso-sortingoption.Why is this necessary?
Closes #4