Skip to content
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 expand-all-hidden-comments feature #2754

Merged

Conversation

kevin940726
Copy link
Contributor

@kevin940726 kevin940726 commented Feb 4, 2020

Closes #1892

Add expand-all-hidden-comments feature. Doing a Alt + Click on the Load more button will load all the hidden comments in the same page. Currently we just recursively click on the button over and over again until there is no more button. We use an undocumented API from #2625 to load all the comments. Should we add a maximum limit on how many pages a click should load?

Test

rust-lang/rfcs#2544

GIF: (pay attention to the scroll bar on the right, it takes a long time...)

Kapture 2020-02-05 at 17 42 43

@fregante
Copy link
Member

fregante commented Feb 4, 2020

Thank you for the PR! #2625 implemented this in a single HTTP request, could you copy it and adjust it to work on alt+click?

@fregante
Copy link
Member

fregante commented Feb 4, 2020

Also, maybe it would make sense to add this as an extra feature of toggle-everything-with-alt

@kevin940726
Copy link
Contributor Author

@fregante I'm a little bit hesitate of implementing it with an official API. Even though everything we did here would potentially break at any time, at least we simulate a regular user interaction as much as possible. However, undocumented API is not meant to be call by the users in any way, especially when we don't know what's the limit of that API. Is there a maximum count on variables[first]? Is there timeouts for that API? Would it be considered malicious in Github logs? Maybe we could ping Github folks for permission though 🤔?

Anyways, that's just my concern. Maybe I worry too much 😂

Also, maybe it would make sense to add this as an extra feature of toggle-everything-with-alt

Ahh, I didn't know we have that! I'll take a look, thx!

@fregante
Copy link
Member

fregante commented Feb 5, 2020

If it breaks in the future, not a huge deal. This feature is enable via alt+click so it doesn’t break regular usage. I’d rather use that than making multiple HTTP calls.

@kevin940726
Copy link
Contributor Author

@fregante Cool, sure! Let me do that later 👍

@kevin940726
Copy link
Contributor Author

@fregante All done! Not sure we should include this in toggle-everything-with-alt though. If it should then probably expand-all-collapsed-code should too 🤔 ?

@fregante
Copy link
Member

fregante commented Feb 5, 2020

@fregante All done! Not sure we should include this in toggle-everything-with-alt though. If it should then probably expand-all-collapsed-code should too 🤔 ?

Agree. Let's merge everything.

Kidding! heheh it's good as is.

source/features/expand-all-hidden-comments.ts Outdated Show resolved Hide resolved
source/features/expand-all-hidden-comments.ts Outdated Show resolved Hide resolved
source/features/expand-all-hidden-comments.ts Outdated Show resolved Hide resolved
source/features/expand-all-hidden-comments.ts Outdated Show resolved Hide resolved
source/features/expand-all-hidden-comments.ts Outdated Show resolved Hide resolved
source/features/expand-all-hidden-comments.ts Outdated Show resolved Hide resolved
@fregante fregante merged commit 248c090 into refined-github:master Feb 7, 2020
@fregante
Copy link
Member

fregante commented Feb 7, 2020

謝謝凱

@shrit shrit mentioned this pull request Sep 13, 2020
@fregante fregante mentioned this pull request Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Add way to load *all* comments when some are hidden due to number of comments
2 participants