-
Notifications
You must be signed in to change notification settings - Fork 10
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
Implements grammyjs/menu#26 #27
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.
Thanks for working on this! I have a few nitpicks that affect edge cases. I would like to see this merged after addressing them:
url?: MaybeDynamicString<C>; | ||
switch_inline_query?: MaybeDynamicString<C>; | ||
switch_inline_query_current_chat?: MaybeDynamicString<C>; |
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.
This is not necessary, these things are already present in the types.
btn.url = btn.url && (await uniform(ctx, btn.url)) | ||
btn.switch_inline_query = | ||
btn.switch_inline_query && | ||
(await uniform(ctx, btn.switch_inline_query_current_chat)) | ||
btn.switch_inline_query_current_chat = | ||
btn.switch_inline_query_current_chat && | ||
(await uniform(ctx, btn.switch_inline_query_current_chat)) |
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.
You are setting these values on the object even when the properties are absent. This is undesired. Have a look at how middleware is handled, and replicate this pattern:
if("url" in btn") {
// ...
}
Are you still working on this? |
I'm closing this due to inactivity. Please reopen if you ever feel like continuing your work. It was a good start. If anyone else is interested in picking this up, please leave a quick comment, too. |
Closes #26