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

Allow bookmarking a message from a context menu #1089

Closed
shenanigansd opened this issue Aug 19, 2022 · 18 comments · Fixed by #1176
Closed

Allow bookmarking a message from a context menu #1089

shenanigansd opened this issue Aug 19, 2022 · 18 comments · Fixed by #1176
Assignees
Labels
status: approved The issue has received a core developer's approval type: enhancement Changes or improvements to existing features

Comments

@shenanigansd
Copy link
Member

shenanigansd commented Aug 19, 2022

We would like to update the existing bookmark command to also be available from the context menu.
#dev-contrib discussion here: https://discord.com/channels/267624335836053506/635950537262759947/1010024284971864104

@shenanigansd shenanigansd added status: planning Discussing details type: enhancement Changes or improvements to existing features labels Aug 19, 2022
@Robin5605
Copy link
Contributor

Maybe not necessarily update, but have the regular text-based bookmark command and the context menu one working side-by-side. I think this one will have to wait until #1087 is merged just to avoid merge conflicts or otherwise multiple PRs for the same feature with potentially conflicting code

@wookie184
Copy link
Contributor

This seems fine to me, is the plan to use ephemeral messages when it's invoked or treat it exactly the same as the command invocation? Ephemeral sounds like it would be neater otherwise it would come out of nowhere.

@wookie184 wookie184 added status: approved The issue has received a core developer's approval and removed status: planning Discussing details labels Sep 10, 2022
@minalike
Copy link
Member

I like the ephemeral response route for this.

@shtlrs
Copy link
Member

shtlrs commented Dec 21, 2022

Why isn't this receiving any love?
I see the PR you mentioned has been Merged @Robin5605 .

I can yoink this and make our lives better!

@Robin5605
Copy link
Contributor

Robin5605 commented Dec 21, 2022

@shtlrs you can feel free to work on it but I can be a backup if needed.
I've worked on similar code with the eval context menu one and it's quite similar so I should be able to do it as well.
So I just want to save you the headache beforehand: you can't have have a context menu decorator inside of a cog. See this comment for more information.

As for making it ephemeral, I'm not sure I like making the resultant method ephemeral because we lose some functionality.
Currently, a user can run the !bookmark command, and an embed is sent with a button, just like we want it. Now, other users can also click on this button and get their very own bookmark sent to the same message without having to run the !bm message again themselves. If we make this ephemeral, other users can't do this. I think being able to see that a certain message was bookmarked may hint that said message contains important information to be filed away for later, and provides an easy outlet for other members to see that and get their own bookmark.

@shtlrs
Copy link
Member

shtlrs commented Dec 21, 2022

No I definitely see your point concerning the ephemeral part.
However, the "definition" of important here is debatable, but also the reach of the message, meaning where it was invoked.
If this were to be in py general for example, and that's because a lot of people engage there ( or in other topical channels as well)
But this can't always be the case in forum help posts, and having a random message popup could be a bit confusing (just a tiny bit IMO)
I struggle to position myself with whether it should ephemeral or not tbh, but I think it wouldn't hurt if it wasn't.

@Robin5605
Copy link
Contributor

Looks like the general consensus is to keep it ephemeral. If, for some reason, we change out minds at a later date, it's an easy fix.
@shtlrs you can go ahead and self-assign now 🙂

@ChrisLovering
Copy link
Member

ChrisLovering commented Dec 21, 2022

The only reason we added the emoji (now button) was to stop multiple people running the command themselves to bookmark a "good" message and end up spamming the channel with bot responses.

Now that ephemeral messages and context menus exist, I am happy to entirely remove the command portion of it and just have the context menu, as it's far more intuitive and doesn't disrupt the conversation.

@shtlrs shtlrs self-assigned this Dec 22, 2022
@shtlrs
Copy link
Member

shtlrs commented Dec 22, 2022

Alright, I'll take this then

@shtlrs
Copy link
Member

shtlrs commented Dec 23, 2022

@Robin5605 @ChrisLovering @shenanigansd
It's sad to see that a user loses the possibility of choosing a title for the bookmark.
What do you think about upon interaction, the bot prompts the user for a title, and sends a bookmark for it? I know it may not be the best experience, but I think it's a shame to lose the title feature

@shenanigansd
Copy link
Member Author

It could be a bit annoying for the people that don't use the feature now to suddenly have it be required, but I think that enough of the userbase uses to make it worth implementing.

@Robin5605
Copy link
Contributor

I think the experience for the majority of users who don't often give it a title should be prioritized.
We want it to be a simple right click > bookmark > done experience.
I'm thinking something like a button attached to the ephemeral embed that opens up a modal allowing you to further edit the title.
This way only people who want to have a title for their bookmark embed have to go through this modal and we don't force everyone to.
I offer my services for this code as it could be potentially complex and I was also the author of discord.py's modal example so I have some experience working with it.
As always, I give first preference if @shtlrs or someone else wants to work on it 🙂

@minalike
Copy link
Member

If we offer the optional ephemeral with modal, how will we time the sending of the DM?

I don't want to sacrifice the ability of adding titles.

@shtlrs
Copy link
Member

shtlrs commented Dec 24, 2022

@Robin5605 Yes that's the original idea.
What you said would work just fine. I'm thinking the user gets a text input that comes with 2 buttons, a '' submit '' one and a '' i don't want to give this a title one'' where the whole operation can be skipped.
It's just the raw idea of course, and the wording is just there as a placeholder.

@minalike when it comes to timing it, here's how i see it.
The user utilizes the context menu, and gets prompted for a title through a view that'll have the previously mentioned components.
This prompt will of course have a timeout that can i agree on, I'm thinking something short like a 15 seconds thing.
If the interaction timeout is reached, or the user willingly skips it, we send the dm with a default title.
Otherwise, we take that input and use it.
WDYT?

@ChrisLovering
Copy link
Member

ChrisLovering commented Dec 24, 2022

I don't think having a timeout would be a good idea, someone might take a little longer to type it out and then it would get cancelled.

I do like the idea of a modal with a text input though.

I think this would work best if we had an optional text input, where the placeholder is the title it would be by default.

That way a user can just submit the modal and get the bookmark with the default title, or overwrite it if they want to.

@shtlrs
Copy link
Member

shtlrs commented Dec 24, 2022

Well, that's actually a great idea as well.

Especially because the message is ephemeral, so if someone doesn't submit the choice, they just wouldn't get the bookmark, but it also won't flood the channel.

I think I'll go for that, and if anyone else disagrees, we can still discuss it of course.

@Robin5605
Copy link
Contributor

Robin5605 commented Dec 24, 2022

Looks like I didn't communicate my idea well -
So a user would click on the bookmark context menu, and immediately the bookmark would be sent to their DM with the default title. The people who don't care about custom titles are happy - nothing has changed for them.

Now, there is an extra button in the ephemeral message in the channel where it was invoked "click here to change the title". This button brings up a modal where you can enter your title, and on submit, the bot would go back and edit the embed in the DM.

This way the process for regular users is not changed at all and no timeouts are involved

@ChrisLovering
Copy link
Member

That seems reasonable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: approved The issue has received a core developer's approval type: enhancement Changes or improvements to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants