-
Notifications
You must be signed in to change notification settings - Fork 580
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
Reverse order of comments #222
base: master
Are you sure you want to change the base?
Conversation
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.
Looks great to me!
Whats the status with this PR? Thats a really great feature |
@jdanyow Any updates on the status of this PR? |
@tpietruszka Any chance you know when we can get this pushed? |
Unfortunately, I do not know anything other than what's in this PR I've sent a review request. |
Would prefer this very much. |
Does anyone know of a maintained fork of utterances with this feature? |
Unfortunately the hosted utterances app (the bot user) is an essential part of this service, so a random fork will not work unless you self-host the server-side part. |
Can this get merged at any point? |
Hope this PR can be merged soon. |
when will this get merged? |
Utterances is unmaintained. Use laymonage/giscus |
I don’t think discussions are going to use reversed order either. What is the benefit of reversing comment order? |
Could you please elaborate? "Either", as in something else won't use it? 🤔 The idea is to have an option of having
instead of
In my application that would provide a better layout and user experience. Judging by the number of comments above it's not a unique case |
I agree. It makes a lot of sense to both sort comments by new and also have the input box sit at the top. This has been my biggest personal hesitation to using this widget. |
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.
This "PR" is not complete.
When the number of pages is exceeded, the "Load more..." button will be displayed.
So we must confirm the processing in the following two places:
- The display position of the "Load more..." button.
- When the "Load more..." button is clicked.
I found the place that needs to be added and modified:
utterances/src/timeline-component.ts
Lines 81 to 91 in 1305da4
public insertPageLoader(insertAfter: IssueComment, count: number, callback: () => void) { | |
const { element: insertAfterElement } = this.timeline.find(x => x.comment.id >= insertAfter.id)!; | |
insertAfterElement.insertAdjacentHTML('afterend', ` | |
<div class="page-loader"> | |
<div class="zigzag"></div> | |
<button type="button" class="btn btn-outline btn-large"> | |
${count} hidden items<br/> | |
<span>Load more...</span> | |
</button> | |
</div> | |
`); |
Make the following changes here:
const desc = page.commentOrder === 'desc';
const { element: insertAfterElement } = this.timeline.find((x) => {
if (page.commentOrder === 'desc') {
return x.comment.id <= insertAfter.id;
} else {
return x.comment.id >= insertAfter.id;
}
})!;
const insert = desc ? 'beforebegin' : 'afterend';
insertAfterElement.insertAdjacentHTML(
insert,
`
<div class="page-loader">……
);
const sibling = desc ? 'previousElementSibling' : 'nextElementSibling';
const element = insertAfterElement[sibling]!;
const indexSearchCondition = (x => x.comment.id >= comment.id); | ||
} | ||
|
||
const index = this.timeline.findIndex(indexSearchCondition); |
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.
const index = this.timeline.findIndex((x) => {
if (page.reverseOrder) {
return x.comment.id <= comment.id;
} else {
return x.comment.id >= comment.id;
}
});
is better than
if (...) {
const indexSearchCondition = ...
} else {
const indexSearchCondition = ...
}
use indexSearchCondition...
<input type="checkbox" id="reverse-order" /> | ||
Reverse order, input form on top | ||
</label> | ||
|
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.
Why not use "reverse-order" and "input-position" as "HTMLSelectElement"?
Isn't that better?
E.g
<h3 id="heading-order">Select order of comments</h3>
<p>
The order of presentation of comments is sorted by the time when the comments were published.
<br />
Ascending order: From the old to the new, the newly posted comments are at the back; Descending order: from the new to the old, and the newly posted comments are at the front.
</p>
<fieldset>
<div>
<label for="comment-order">
By time:
</label>
<br />
<select id="comment-order" class="form-select" value="asc" aria-label="Comment order">
<option value="asc">Asc(default)</option>
<option value="desc">Desc</option>
</select>
</div>
</fieldset>
<h3 id="heading-input-position">Select comment box location</h3>
<p>
When choosing to set the comment order to "descending", it is recommended to place the comment box at the "top". Because when there are a lot of comments, you can see the success of the comment as soon as you post a comment.
</p>
<fieldset>
<div>
<label for="input-position">
The comment box's position at:
</label>
<br />
<select id="input-position" class="form-select" value="asc" aria-label="Comment order">
<option value="bottom">Bottom (default)</option>
<option value="top">Top</option>
</select>
</div>
</fieldset>
this.script.innerHTML = this.makeConfigScript( | ||
this.makeConfigScriptAttribute('repo', this.repo.value === '' ? '[ENTER REPO HERE]' : this.repo.value) + '\n' + | ||
mappingAttr + '\n' + | ||
(this.label.value ? this.makeConfigScriptAttribute('label', this.label.value) + '\n' : '') + | ||
this.makeConfigScriptAttribute('theme', this.theme.value) + '\n' + | ||
orderConfig + |
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.
(this.commentOrder.value === 'desc'
? this.makeConfigScriptAttribute(
'comment-order',
this.commentOrder.value,
) + '\n'
: '') +
(this.inputPosition.value === 'top'
? this.makeConfigScriptAttribute('input-position', 'top') + '\n'
: '') +
Occam's razor: if not necessary, not by entity.
theme: params.theme || 'github-light' | ||
theme: params.theme || 'github-light', | ||
inputPositionTop: inputPositionTop, | ||
reverseOrder: reverseOrder |
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.
commentOrder: params['comment-order'] || 'asc',
inputPosition: params['input-position'] || 'bottom',
Occam's razor: if not necessary, not by entity.
@zsdycs good comments and improvements, thanks Please feel free to branch off of this and open your own pull request. (or one starting from the current master branch?) To be honest, after over a year of no response (and then a response I did not really understand, but one that sounded like the feature is not needed) I kind of lost interest. |
@tpietruszka Thank you for your approval! This feature is very good, I think many people expect to be able to use this feature. The purpose of my proposed modification is to hope that developers who are interested in this feature will have more reference information. So, unfortunately, I will not submit the changed code in this branch or from the master branch, because I think this is futile, unless @jdanyow agrees with this feature, at least he is not interested in it for now. But we have to understand that "utterances" is currently a mature plug-in, and we must consider many aspects when introducing a feature. The most important thing is that I use this code. After I modified it, I merged it into the Chinese version of "utterances"-"beaudar - 表达". Thank you for your contribution! |
so any update on this ? |
Closes #182
Added options to reverse order of comments and put the input form above comments, instead of below. As most of the time I expect either both or none of them to be used, they are handled by a common checkbox in the config generator.
Overall it seems to work, but I have not tested it on cases with hidden pages (I have not seen any).
Example config (from the
configuration-component
):