Skip to content

Use buttons for the bookmark command#1091

Closed
Diabolical5777 wants to merge 3 commits into
python-discord:mainfrom
Diabolical5777:main
Closed

Use buttons for the bookmark command#1091
Diabolical5777 wants to merge 3 commits into
python-discord:mainfrom
Diabolical5777:main

Conversation

@Diabolical5777
Copy link
Copy Markdown
Contributor

@Diabolical5777 Diabolical5777 commented Aug 20, 2022

Relevant Issues

Closes #932

Description

I replaced the reactions used currently with buttons

Did you:

Comment thread bot/exts/utilities/bookmark.py Outdated
Comment thread bot/exts/utilities/bookmark.py Outdated
Diabolical5777 and others added 2 commits August 20, 2022 18:25
Co-authored-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com>
Co-authored-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com>
@Robin5605
Copy link
Copy Markdown
Contributor

It looks like we have a full on discord.ui.View subclass for a simple link button - but for the actual button that sends the bookmark in the DM, we're just doing:

view = discord.ui.View(timeout=TIMEOUT)
view.add_item(
    SendBookmark(self.action_bookmark, ctx.author, ctx.channel, target_message)
)

I think this should be flipped around. There's no need for a full fledged subclass for a simple link button, but maybe we can use a view subclass for the actual bookmark. I doubt we'll also need a button subclass since we're not doing anything dynamic

@Diabolical5777
Copy link
Copy Markdown
Contributor Author

It looks like we have a full on discord.ui.View subclass for a simple link button - but for the actual button that sends the bookmark in the DM, we're just doing:

view = discord.ui.View(timeout=TIMEOUT)
view.add_item(
    SendBookmark(self.action_bookmark, ctx.author, ctx.channel, target_message)
)

I think this should be flipped around. There's no need for a full fledged subclass for a simple link button, but maybe we can use a view subclass for the actual bookmark. I doubt we'll also need a button subclass since we're not doing anything dynamic

I think you have it flipped. I used the button subclass for the button that is sent in the channel where the bookmark is invoked in so others can press the button. For the button in DM's that redirects to the bookmarked message, I used a view directly.

@Xithrius Xithrius added area: backend Related to internal functionality and utilities status: needs review Author is waiting for someone to review and approve type: enhancement Changes or improvements to existing features category: utilities Related to utilities labels Aug 24, 2022
@Xithrius Xithrius requested a review from ChrisLovering August 24, 2022 22:24
@Robin5605
Copy link
Copy Markdown
Contributor

You're right that I had it flipped. The only other thing I might recommend we change is maybe subclassing discord.ui.View instead of a discord.ui.Button? Usually that's what we do for non-dynamic components. And I think all the parameters we pass into SendBookmark can be passed into the view subclass, and we shouldn't lose any functionality

@Diabolical5777 Diabolical5777 closed this by deleting the head repository Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: backend Related to internal functionality and utilities category: utilities Related to utilities status: needs review Author is waiting for someone to review and approve type: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace reactions in the bookmark command with buttons.

4 participants