search

etcet / HNES

Hacker News Extension Suite
Other
248 stars 71 forks source link

Implement persistent collapsed comments #87

Closed ibejoeb closed 9 years ago

ibejoeb commented 9 years ago

Add persistent collapsed comments to resolve #84.

This also adds an abstract comment tree that can be used to drive other features.

ibejoeb commented 9 years ago

This also corrects (i.e., resists breaking changes giving rise to) issue #88.

@etcet : HNES is currently broken, as far as I can tell. PR #90 fixes this also and is a very small patch. It'd be nice to merge it if you don't want to go as far as I've gone. However, I'm interested in continuing to improve it, so I hope you'll consider this PR.

etcet commented 9 years ago

First off: thanks for working on this and I'm sorry I waited so long. I hate to ask this but could you fix the current conflicts?

There's the SVG arrows in https://github.com/etcet/HNES/pull/91 which we should keep and https://github.com/etcet/HNES/pull/90 which your code doesn't use.

One thing I noticed when testing this is dead comments can't be collapsed. There's one on https://news.ycombinator.com/item?id=10339388 if you search for "[deleted]". Also, if you collapse the parent the dead comment remains visible.

ibejoeb commented 9 years ago

Yes, let me take a look. I haven't seen the upstream code since this was made.

I don't seem to be experiencing the collapsed dead problems, but I'll try on some other items.

Also, it looks like there is some history manipulation on master, so it might take me a bit to reincorporate this stuff.

etcet commented 9 years ago

Thanks Joe. I've manually merged a pull request before but Github didn't give credit to the author as it did when it can be automatically merged. I don't want that to happen to you. This isn't time sensitive so please don't feel rushed. I promise to merge as soon as you're ready.

ibejoeb commented 9 years ago

I've done the majority of the merge. I've incorporate #91.

On #90, what do you mean by "we should keep" it? I implement it in CSS, which was resilient to the breakage anyway, as described here: https://github.com/etcet/HNES/pull/87#issuecomment-120775391. Are you saying that you'd rather keep the jQuery implementation? Or do you just want to preserve the exact styling, which looks like "[−] ckozlowski 10 days ago | parent" I can definitely tweak mine to preserve that.

ibejoeb commented 9 years ago

Had to do a manual merged. This PR is superseded by https://github.com/etcet/HNES/pull/98.