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

Change checkin args #45

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Change checkin args #45

merged 4 commits into from
Oct 24, 2023

Conversation

Stuart6557
Copy link
Contributor

  • Changed 'now' parameter to 'public', false by default
  • Removed filtering for ongoing events only

Copy link
Contributor

@alexzhang1618 alexzhang1618 left a comment

Choose a reason for hiding this comment

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

Changes are looking good! Some comments/questions before I think we can merge/deploy:

src/Client.ts Outdated
Comment on lines 48 to 55
'GUILDS',
'GUILD_INTEGRATIONS',
'GUILD_WEBHOOKS',
'GUILD_MESSAGES',
'GUILD_MEMBERS',
'DIRECT_MESSAGES',
'GUILD_MESSAGE_REACTIONS',
'DIRECT_MESSAGE_REACTIONS',
GatewayIntentBits.Guilds,
GatewayIntentBits.GuildIntegrations,
GatewayIntentBits.GuildWebhooks,
GatewayIntentBits.GuildMessages,
GatewayIntentBits.DirectMessages,
GatewayIntentBits.GuildMessageReactions,
GatewayIntentBits.DirectMessageReactions,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these changing in this PR? Does it work without these changes?

* the widescreen slide QR (true).
* This Command DM's the caller the checkin code and Express Checkin link for any current and
* upcoming events in today's timeframe. Optional argument `public` makes the embed with the
* checkin codes be returned in the same chat as the Command message instead of DMs. Optinoal
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
* checkin codes be returned in the same chat as the Command message instead of DMs. Optinoal
* checkin codes be returned in the same chat as the Command message instead of DMs. Optional

@@ -40,7 +40,7 @@ export default class Checkin extends Command {
boardRequired: true,
enabled: true,
description:
"Sends a private message with all check-in codes from today's events. Calling with `now` argument sends public embed of checkin code if any events are now live!",
"Sends a private message with all check-in codes from today's events. Calling with `public` argument sends public embed of checkin code.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe here it should specify that public sends in the current channel instead of via DM?

@@ -85,10 +86,6 @@ export default class Checkin extends Command {
//
// We need two sets of arrays for "checkin":
Copy link
Contributor

Choose a reason for hiding this comment

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

Can update this comment and related ones: we don't need two sets of arrays for "checkin" anymore

Comment on lines 12 to 19
'GUILDS',
'GUILD_INTEGRATIONS',
'GUILD_WEBHOOKS',
'GUILD_MESSAGES',
'GUILD_MEMBERS',
'DIRECT_MESSAGES',
'GUILD_MESSAGE_REACTIONS',
'DIRECT_MESSAGE_REACTIONS',
GatewayIntentBits.Guilds,
GatewayIntentBits.GuildIntegrations,
GatewayIntentBits.GuildWebhooks,
GatewayIntentBits.GuildMessages,
GatewayIntentBits.DirectMessages,
GatewayIntentBits.GuildMessageReactions,
GatewayIntentBits.DirectMessageReactions,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as in Client.ts

Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be removed!

Copy link
Contributor

@alexzhang1618 alexzhang1618 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@Stuart6557 Stuart6557 merged commit d828692 into master Oct 24, 2023
2 checks passed
@Stuart6557 Stuart6557 deleted the change-checkin-args branch October 24, 2023 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants