plibither8 / refined-hacker-news

✨ Hacker News, but refined — Interface tweaks and features to make the HN experience better
MIT License
808 stars 34 forks source link

Feature request: Toggle child comment's root #61

Closed jvzr closed 5 years ago

jvzr commented 5 years ago

Hi @plibither8

Quick thank you for your work, I've found this extension to be a great help navigating HN comments.

I've made a few CSS changes which fix some issues I had (see screenshot below):

image

There is one feature I miss from a previous userscript: the ability to collapse all comments until the root of the tree of the current comment. For illustration purpose:

If I click on this new 'collapse parents' on comment 1.2, it would collapse all Comments 1, 1.1, 1.2. Ditto for 2.1.1 and 2, 2.1 I hope this is clear :)

In my vision, this feature would either be a button at the right of HN's default collapse button, or the top n pixels of the clickable side rail you've added yourself.

image

I figure I could hack something and submit a PR but I'm not quite sure I have the capacities to incorporate into your system. I suppose I'd have to getTopLevelComments() and find somehow the one that is the current comment's grand-parent, and then toggle that comment through its toggle button

Anyway, if you can show me some pointers I might be able to push something. Otherwise I'm available if you have questions :)

plibither8 commented 5 years ago

Hi Jerry, thanks for the kind words :)

I've made a few CSS changes which fix some issues I had (see screenshot below):

  • easier to collapse top-level comment
  • easier to see at which indent level is the comment I'm reading

I'd really like to know more about the CSS changes that you incorporated - maybe I can incorporate them as a native feature the extension provides.

If I click on this new 'collapse parents' on comment 1.2, it would collapse all Comments 1, 1.1, 1.2. Ditto for 2.1.1 and 2, 2.1

If I understand correctly, all comments above having an indent less than or equal to the comment I click "collapse parents" on will collapse. To clarify, if the comment tree is like so:

If I click "collapse parents" on C1.2, this should collapse C1 thereby (implicitly) collapsing C1.1, C1.2, C1.2.1 and C1.3 too. I hope this is what you mean.

If it is so, the implementation of it should be fairly simple. We can first identify the ID of the target comment (the comment that got clicked on) and find it's position in the Array getAllComments(). Then we can employ a simple while loop over the getAllComments() array the checks each comment above/before the target comment and sees whether the indent amount is '0' or not (indicating a top-level comment); see this: https://github.com/plibither8/refined-hacker-news/blob/f6290082b1d9927cdaca9831bb7e69b730cb6617/src/libs/handle-item-keydowns.js#L11. Once the top-level comment has been identified, we can trigger a .click() on its collapse button.

A button on the right of HN's default collapse button would be simpler and easier-to-use/understand method of doing so, in my opinion.

jvzr commented 5 years ago

Hey Mihir,

Thank you for answering blazingly fast!

You'll find my CSS here: https://gist.github.com/jvzr/ec0a5dfa66ff178f4ce01e17f0a2bb01 Some of it is cosmetic preferences, and the rest is to further your work and make the 'collapse rail' as I call it, more visible. I have bad eyes.

Regarding the 'collapse grandparent' feature, this is exactly what you described! Here's my status: https://gist.github.com/jvzr/8f3de3e70b141f97ffb86fcd0dc4fcd4 Keep in mind that I'm a hacker in the most candid sense: I hack things together. I have no CS education at all

I figured I needed to add the button to collapse grandparent on all comments at initialization. I figured also that I should not put that on a grandparent (or root, or indent level 0, whatever name) but I could instead keep an array of their position/index in the getAllComments() array. This way, I would be able to get the index of the current comment, check the closest-but-lesser index from that GP array, and collapse that one.

Does that make sense or am I complicating things?

Also, how would I go about testing that? I don't see a way to test it from the console since the utilities are scoped to your extension and not available in the console.

plibither8 commented 5 years ago

I'm on mobile right now, so I won't be able to test the custom CSS - I'll reply regarding that separately.

A minor suggestion - let's call the "grandparent" comment the "root" comment instead. It semantically more sense in my opinion. So instead of "Collapse GP" we can have "collapse root comment" or "collapse root" (also keeping it consistent with other HN links, the inner text should ideally be lowercase :) ).

As for the implementation - I think I've figured a much simpler and more efficient implementation. We can maintain a currentRootComment variable that holds the root comment's element. It gets assigned every time the indent level is 0. And for each child comment we are looping over, we can get root comment from this dynamic variable. A possible implementation:

const comments = getAllComments();
let currentRootComment;

for (const comment of comments) {

    const indentLevel = getCommentIndentation(comment);
    if (indentLevel === 0) {
        currentRootComment = comment;
        continue;
    }

    const toggle = document.createElement('a');

    toggle.innerText = 'collapse root';
    toggle.classList.add('__rhn__collapse-grandparent-comment');
    toggle.addEventListener('click', () => {
        // find comment's root and click its toggle button
        currentRootComment.querySelector('a.togg').click();
    });

    comment.querySelector('span.comhead').append(toggle);
}

For testing, it'd be great if you could fork this repo, clone it on your machine and proceed with the development. To start, you could read this: https://github.com/plibither8/refined-hacker-news/blob/master/CONTRIBUTING.md. If you have any queries, please do ask :). Also note, you'll have to disable the currently installed RHN so as to make it work properly.

jvzr commented 5 years ago

Hey Mihir,

Excellent suggestion, you mean! Approved 👍

Your implementation is much better indeed. It just needed to 'instantiate' in a way the value of currentRootComment in the for..of loop, otherwise they all ended up with the last root comment.

I got it all working* in Chrome. I haven't had the guts to test it on Firefox (even though that's the browser I use) but I don't see how it would not work. What shall I do next? I suppose I need to commit, and somehow send a pull request? How would I do that? This is my first contribution...

Thanks for all the help!

*this was quite a journey. Had to install WSL, Ubuntu, NPM, all the dependencies, etc. Your readme was quite helpful!

PS: now I realize I need to add a way to scroll to the collapsed comment, otherwise it's easy to get lost PPS: that's been added now

plibither8 commented 5 years ago

Hi!

this was quite a journey. Had to install WSL, Ubuntu, NPM, all the dependencies, etc. Your readme was quite helpful!

I'm so glad that you went along with it all - I can understand the trouble, I'm grateful :)

Alright! So let's get started with the pull request. I assume that since you have installed WSL and Ubuntu now, you have git installed too. Otherwise: sudo apt install git should do it.

In the terminal, cd into the project directory. We want to create a new feature branch with this new feature. This is how it's done: git checkout -b <branch-name> and replace with the actual name of the branch, maybe something like "feat/collapse-root-comment".

Now we stage the files that we have added/modified and commit them:

git add .
git commit -m "Your Commit Message" 

...and replace "Your Commit Message" with the actual commit message describing what change this commit brings about in under ~50 chars.

It's time to push this branch and commit to the remote repo - your fork of RHN on GitHub:

git push origin <branch-name>

Again, replace branch-name with the actual name of the branch we had decided before. This should now ask for your GitHub username and password. After entering them successfully the files and changes will get uploaded onto GitHub.

Now it's time to create the Pull Request! Go here: https://github.com/jvzr/refined-hacker-news/pull/new/master. Select the branch from the "compare..." dropdown button. This will nwo check whether the branch is merge-able, and if it is, you will be able to create a new pull request by clicking that big green button.

I hope I've explained the process as clearly as possible. If there are queries, please do ask! Thanks again :)

plibither8 commented 5 years ago

From what I see, I don't think you have, till now, forked this repository to your GitHub account. All of the above instructions will only work if you first fork this repository, and then git clone your forked repository. If you require elaboration on this, I'd be more than happy.

jvzr commented 5 years ago

Excellent instructions, thank you again!

I hope I followed them well. You should be seeing the new pull request now :)