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

401 add pagination helper #527

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

sagarpatel211
Copy link

Summary of Changes

  • This ticket addresses the implementation for an efficient pagination helper function.

Motivation and Explanation

  • This feature improve the bot as it expands functionality, such as displaying long paragraphs of text in a paginated way

Related Issues

Steps to Reproduce

  • Run the discord bot and checkout to branch 401-add-pagination-helper
  • Call .pg, .pagination, or .pgtest in your test channel to try out an example pagination I wrote

Demonstration of Changes

  • A sample demo for using pagination is provided through src/commands/miscellaneous/pagination-test.ts and src/commandDetails/miscellaneous/pagination-test.ts, which can be removed.

Further Information and Comments

  • Let me know if there's any changes I can make or improvements in my code style, documentation, etc.!

@sagarpatel211 sagarpatel211 linked an issue Jun 5, 2024 that may be closed by this pull request
Copy link
Contributor

@probro27 probro27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sagarpatel211 Looks really good. My only issue with this is that the buttons tend to disappear after a while, leaving the embed stuck on a particular page.
image
Is there a way to fix this?

@hyperneutrino
Copy link

@probro27 if we want to avoid memory leaks we cannot keep the buttons around forever (except in cases where the page can be generated from the page number without needing to store the embeds in RAM, but that seems like a future enhancement). maybe to improve the intentionality we can disable the buttons before quitting the listener so it's more obvious that it was paginated and it was intentionally disabled, but that's an aesthetic decision

Copy link
Contributor

@probro27 probro27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sagarpatel211 Based on the comment by Alex, the only thing we need to add is a new function that overloads the existing one, except it takes the entire text or leaderboard data and divides it into pages itself.
We can keep the current function which is useful for specific page data.
Thanks!

@sagarpatel211
Copy link
Author

sagarpatel211 commented Jun 19, 2024

New changes include:

  • A default timeout of 300 seconds (5 min).
  • A default max page count of 25.
  • A default max newlines (ie. line height) of 10
  • A default max character count per page of 2048.
  • Navigator buttons disable for 3 seconds before disappearing for aesthetics.
  • A new PaginationBuilderFromText function which allows for large multi-line strings as input, separating pages by newlines or reaching the max character count. I decided to have separate function names between the Embed list input one and text input one for clarity since it's an internal helper function. Pages using text as input have random-coloured embeds. Option to disable newlines page separation for better leaderboard support.

Let me know if there's more suggestions. If not, after removing the pagination-test command it can be merged.

@sagarpatel211 sagarpatel211 self-assigned this Jul 28, 2024
@KuroganeToyama KuroganeToyama requested review from KuroganeToyama and davidgan1218 and removed request for Fan-Yang-284 September 29, 2024 02:26
@davidgan1218
Copy link
Contributor

Only the user who invokes the pagination should be able to see the buttons to change pages. Right now, pressing the buttons as an "outside" user results in a "This interaction failed" message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Pagination Helper
4 participants