-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: chain log #276
base: master
Are you sure you want to change the base?
feat: chain log #276
Conversation
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.
Looks good! A few things.
-
Clarify what the question is.
Perhaps have the question be "In-Game Name of Key Popper." I was kind of confused when I initially saw it.
-
In the control panel, it might be a good idea to update the instructions (and corresponding button text) to include the Chain log button.
-
I've been getting some of these randomly:
DiscordAPIError: Interaction has already been acknowledged. at RequestHandler.execute (ToogaBooga\node_modules\discord.js\src\rest\RequestHandler.js:350:13) at process.processTicksAndRejections (node:internal/process/task_queues:95:5) at async RequestHandler.push (ToogaBooga\node_modules\discord.js\src\rest\RequestHandler.js:51:14) at async ModalSubmitInteraction.reply (ToogaBooga\node_modules\discord.js\src\structures\interfaces\InteractionResponses.js:103:5)
See if it's related to your addition of the prompts, but otherwise don't worry too much about it.
-
See review comments in code.
); | ||
|
||
if (!resObj) { | ||
(originalMessage as unknown as Message<true>).delete().catch(e => LOGGER.error(`${this._instanceInfo} ${e}`)); |
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.
Can you leave a comment as to why you're doing all of this casting?
src/instances/RaidInstance.ts
Outdated
|
||
const BUTTONS = new MessageActionRow(); | ||
if (!this._vcless) { | ||
const VC_USERS_BUTTON = new MessageButton() |
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.
Use camelcase here, or define a constant and import the constant. For what it's worth, AdvancedCollector
has a helper function cloneButton
that you might find helpful. Use the function to clone the button and then, if needed, assign a new custom ID.
@@ -1517,6 +1527,234 @@ export class RaidInstance { | |||
}, 5 * 60 * 1000); | |||
} | |||
|
|||
private async provideChainLogModal(i: ButtonInteraction<"cached">): Promise<void> { |
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.
Document what this method does
src/instances/RaidInstance.ts
Outdated
const VC_USERS_BUTTON = new MessageButton() | ||
.setCustomId("chain_log_use_vc") | ||
.setLabel("Users in VC") | ||
.setEmoji("🎙️") |
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.
Maybe define the emoji constant in the EmojiConstants.ts file and then import the constant.
src/instances/RaidInstance.ts
Outdated
|
||
BUTTONS.addComponents(VC_USERS_BUTTON); | ||
} | ||
const SKIP_BUTTON = new MessageButton() |
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.
Use camelcase here, or define a constant and import the constant. For what it's worth, AdvancedCollector
has a helper function cloneButton
that you might find helpful. Use the function to clone the button and then, if needed, assign a new custom ID.
src/instances/RaidInstance.ts
Outdated
@@ -159,6 +164,11 @@ export class RaidInstance { | |||
.setEmoji(EmojiConstants.UNLOCK_EMOJI) | |||
.setCustomId(RaidInstance.UNLOCK_RAID_ID) | |||
.setStyle("PRIMARY"), | |||
new MessageButton() | |||
.setLabel("Chain log") |
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.
To keep things consistent, change this to Chain Log
(capitalize L
).
How to chain log:
Closes #188