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

Convert Id views to using new menu code #750

Merged
merged 11 commits into from
Feb 6, 2021

Conversation

RheingoldRiver
Copy link
Collaborator

Outstanding issues from #741

  • implemented arrows
    • ViewStates now have pane_types
    • they also now record the user's initial scroll preference - this is DEF preferable to re-computing because friends
  • arrows to tabs that lack emojis are ignored right now - they are currently throwing errors which is not great; better would be for Tsubaki to throw up a forbidden sign at the end of the menu and remove your emoji. However, because the menu is stateless, throwing the error doesn't stop the menu from working or cause anything to crash, so you can continue to click other reactions! So it doesn't really matter too much! Hooray! but also see When scrolling support throwing up a no_entry / forbidden emoji at the end briefly (Say 3 seconds) discord-menu#23
  • did a possibly temporary way to set up different starting screens but this could also possibly just be permanent idk

@RheingoldRiver
Copy link
Collaborator Author

Issues I don't like

  1. It doesn't feel "nice" that anything outside of the IdMenu class knows about the IdMenu.evos_control etc functions but idk maybe this is fine, but I don't think this should be permanent way to do differing initial tabs
  2. Can this:
await (get_monster_by_id(dgcog, resolved_monster_id)
                         if resolved_monster_id else get_monster_by_query(dgcog, raw_query, user_config.beta_id3))

be its own function in core/id? Since all it does is resolve via either monster_id or raw_query and it's common to all ViewStates it seems like that could be extracted to a common function reasonably.

padinfo/id_menu.py Outdated Show resolved Hide resolved
padinfo/padinfo.py Outdated Show resolved Hide resolved
padinfo/padinfo.py Outdated Show resolved Hide resolved
padinfo/padinfo.py Outdated Show resolved Hide resolved
padinfo/padinfo.py Outdated Show resolved Hide resolved
padinfo/padinfo.py Outdated Show resolved Hide resolved
@@ -4,6 +4,8 @@
import discord
from discord import Color

from padinfo.core.padinfo_settings import settings
Copy link
Owner

Choose a reason for hiding this comment

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

can this entire file be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's still links and lssingle


def serialize(self):
ret = super().serialize()
ret.update({
'pane_type': 'materials',
Copy link
Owner

Choose a reason for hiding this comment

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

Define a class with string so this isn't hardcoded.

@staticmethod
def get_pane_types():
return {
'id': IdMenu.respond_with_current_id,
Copy link
Owner

Choose a reason for hiding this comment

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

Define a class with strings so the key isn't hardcoded.

@@ -10,34 +10,40 @@

class PicViewState(ViewState):
def __init__(self, original_author_id, menu_type, raw_query, query, color, monster: "MonsterModel",
use_evo_scroll: bool = True,
Copy link
Owner

Choose a reason for hiding this comment

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

why optional

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in case anyone is reading this later - if these views are used outside of the ^id command (which IdViewState is), it's possible we won't care about the use_evo_scroll preference and not have determined that so we don't require it

@TsubakiBotPad TsubakiBotPad merged commit f07bb52 into TsubakiBotPad:master Feb 6, 2021
@RheingoldRiver RheingoldRiver deleted the arrows2 branch February 6, 2021 23:59
chasehult pushed a commit that referenced this pull request Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants