-
-
Notifications
You must be signed in to change notification settings - Fork 104
Modal component added for modmail #715
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
Conversation
|
Can you share some screenshots please? |
Zabuzard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(its just a draft PR, but i thought ill just add some stuff before we forget it)
application/src/main/java/org/togetherjava/tjbot/commands/basic/TestCommand.java
Outdated
Show resolved
Hide resolved
|
I don't believe that this improves UX in any ways, quite the opposite. Having a slash-command option allows it to only allow "true" and "false" There is no error or warning in the modal itself. |
|
I was rather thinking of sth mixed actually. Like, the user pops the slash command with all the options and then gets a modal to input the message. Or similar maybe |
|
Isn't that the current implementation? If no, yes that's best UX we can have |
|
Yeah I can keep the server and stay anonymous as options and then open up a modal for the message only. I will update this branch |
| private static final Logger logger = LoggerFactory.getLogger(ModMailCommand.class); | ||
| public static final String COMMAND_NAME = "modmail"; | ||
| private static final String OPTION_MESSAGE = "message"; | ||
| private static final String MESSAGE = "message"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it has to be clear what this field is and where its used. the current name doesnt tell.
maybe go for MODAL_INPUT_MESSAGE or similar
...ication/src/main/java/org/togetherjava/tjbot/commands/moderation/modmail/ModMailCommand.java
Show resolved
Hide resolved
...ication/src/main/java/org/togetherjava/tjbot/commands/moderation/modmail/ModMailCommand.java
Outdated
Show resolved
Hide resolved
...ication/src/main/java/org/togetherjava/tjbot/commands/moderation/modmail/ModMailCommand.java
Outdated
Show resolved
Hide resolved
...ication/src/main/java/org/togetherjava/tjbot/commands/moderation/modmail/ModMailCommand.java
Outdated
Show resolved
Hide resolved
...ication/src/main/java/org/togetherjava/tjbot/commands/moderation/modmail/ModMailCommand.java
Outdated
Show resolved
Hide resolved
| MessageCreateAction message = | ||
| createModMessage(event, userId, modMailAuditLog.orElseThrow()); | ||
| MessageCreateAction message = createModMessage(event, Boolean.parseBoolean(args.get(0)), | ||
| userId, modMailAuditLog.orElseThrow()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is easier to read if you extract the args seperated and at the beginning:
boolean stayAnonymous = Boolean.parseBoolean(args.get(0));
long guildToContact = Long.parseLong(args.get(1));
...|
i'd say keep the |
|
@kavink98 r u still active, or do u want someone else to take over this pr? |
|
Sorry, this past month has been hectic for me. I'm in the midst of moving
to a new country. I'll be active again in a week's time.
…On Sat, 7 Jan, 2023, 5:42 am Tanish, ***@***.***> wrote:
@kavink98 <https://github.com/kavink98> r u still active, or do u want
someone else to take over this pr?
—
Reply to this email directly, view it on GitHub
<#715 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJZFZNOVR3DQY2FRUZ5LC3TWRFCALANCNFSM6AAAAAASYAW6GU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
This pull request is stale because it has been open 30 days with no activity. Remove stale label, comment or add the valid label or this will be closed in 5 days. |
|
@kavink98 should someone else take over this pr? |
|
done in #800 |

Cannot add lists to modals currently, so the server tab is removed. Anonymous is enabled via a text input where the user has to type in yes if they want to stay anonymous