feat: btn selection for all lookups with servsettings toggle#2199
feat: btn selection for all lookups with servsettings toggle#2199codybanman merged 1 commit intoavrae:nightlyfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@1drturtle Thanks for taking out the time to review the pr, I've pushed another commit that hopefully adresses all the concerns, here's a screenshot of current latest state you'd asked for: |
|
ah thought I linted, one sec will fix these |
|
@codybanman amended the lint fix, pls try tests again whenever able, thank you! |
|
that was a sneaky newline in there, fixed now sorry @codybanman |
codybanman
left a comment
There was a problem hiding this comment.
First off, thanks putting the time and effort into this. It's a cool idea and definitely makes the !i madd experience better.
There are a few issues I’ve noted in the comments. I didn’t have time to go through everything, but this covers a big portion of it. Please take a look and let me know if you have any questions.
utils/selection_views.py
Outdated
| def _create_selection_buttons( | ||
| self, start_idx: int, end_idx: int, current_choices: List[Any], current_page: int, prefix: str | ||
| ) -> None: | ||
| """Create selection buttons for a given range.""" | ||
| for i in range(start_idx, end_idx): | ||
| if i < len(current_choices): | ||
| global_index = i + 1 + current_page * 10 | ||
| button = disnake.ui.Button( | ||
| label=str(global_index), | ||
| style=disnake.ButtonStyle.secondary, | ||
| disabled=self.expired, | ||
| custom_id=f"{prefix}select_{global_index}", | ||
| ) | ||
| else: | ||
| button = disnake.ui.Button( | ||
| label=str(i + 1 + current_page * 10), | ||
| style=disnake.ButtonStyle.secondary, | ||
| disabled=True, | ||
| custom_id=f"{prefix}placeholder_{i}", | ||
| ) | ||
| self.add_item(button) |
There was a problem hiding this comment.
| def _create_selection_buttons( | |
| self, start_idx: int, end_idx: int, current_choices: List[Any], current_page: int, prefix: str | |
| ) -> None: | |
| """Create selection buttons for a given range.""" | |
| for i in range(start_idx, end_idx): | |
| if i < len(current_choices): | |
| global_index = i + 1 + current_page * 10 | |
| button = disnake.ui.Button( | |
| label=str(global_index), | |
| style=disnake.ButtonStyle.secondary, | |
| disabled=self.expired, | |
| custom_id=f"{prefix}select_{global_index}", | |
| ) | |
| else: | |
| button = disnake.ui.Button( | |
| label=str(i + 1 + current_page * 10), | |
| style=disnake.ButtonStyle.secondary, | |
| disabled=True, | |
| custom_id=f"{prefix}placeholder_{i}", | |
| ) | |
| self.add_item(button) | |
| def _create_selection_buttons( | |
| self, start_idx: int, end_idx: int, current_choices: List[Any], current_page: int, prefix: str, row: int | |
| ) -> None: | |
| """Create selection buttons for a given range.""" | |
| for i in range(start_idx, end_idx): | |
| if i < len(current_choices): | |
| global_index = i + 1 + current_page * 10 | |
| self.add_item(disnake.ui.Button( | |
| label=str(global_index), | |
| style=disnake.ButtonStyle.secondary, | |
| disabled=self.expired, | |
| custom_id=f"{prefix}select_{global_index}", | |
| row=row, | |
| ) |
How about using rows to define where the buttons should be rather than dummy buttons and relying on the buttons automatically being put in rows of 5? Set 0-5 as row 0, 5-10 as row 1, and the navigation buttons as row 2.
There was a problem hiding this comment.
About this, the different shapes of btn layouts thrown every time for different selection sizes felt a bit jarring. There was something nice and familiar about always expecting the same layout and shape.
More of a user preference really, maybe this is something that the users can vote on a poll on the discord request discussion channel? Or an announcement poll, even? I don't mind either way, we can just commit this version of hidden btns too
utils/selection_views.py
Outdated
| message: Optional[str] = None, | ||
| pm: bool = False, | ||
| ctx=None, | ||
| is_monster: bool = False, |
There was a problem hiding this comment.
This variable isn't used in the function
|
Thank you for the detailed review @codybanman good catch on the legacy preferences, couldn't test it without beyond data. Will roll out a commit shortly to fix those and the other issues. About hiding the btns and disabling the boundary nav btns, have a few points to discuss which I will note above |
|
Couldn't test the legacy issue without beyond data, so just added unit tests for the legacy checking, pls do test on beyond data and let me know if any issues. Otherwise, hopefully everything is ready to go, you can also add in your changes to hide btns (I started a poll on discord) and just roll out to nightly for testing! Looking forward to seeing this on the main bot soon! |
|
All systems are a GO and ready for launch! Pending tests and review ofc :) |
2b57451 to
fa8e5f1
Compare
799f428 to
0c68413
Compare
5283e8a to
ed7c0ad
Compare
d950463 to
8ec69fe
Compare
EDIT: Below summary is outdated, this now covers all lookups with a servsettings toggle, and enhanced UX during
!i maddthat lets the user click links to change channels instead of manually changing channels via discord's interface.Currently enabled for monster lookups only, with easy expansion to other entity types based on feedback.
Summary
Adds interactive buttons for madd and monster selection instead of just text input. Users can now click numbered buttons or still type numbers like before. Currently only enabled for monsters to test the waters. New file handles the button logic, minimal changes to existing code.
Changelog Entry
Interactive button selection for all selection menus with servsettings toggle
Checklist
PR Type
Other