Skip to content

[Notifier] Updated chatter documentation with Telegram::answerCallbackQuery #17514

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

Merged

Conversation

alexsoft
Copy link
Contributor

@alexsoft alexsoft commented Dec 5, 2022

Adding documentation for the new feature of Telegram notifier: answer callback query.
Fixes #17501

@carsonbot carsonbot added this to the 6.3 milestone Dec 5, 2022
@OskarStark OskarStark changed the title [Notifier] Updated chatter documentation with Telegram::answerCallbackQuery [Notifier] Updated chatter documentation with Telegram::answerCallbackQuery Dec 5, 2022
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Some comments, afterwards good to merge 👍

Comment on lines 335 to 336
Answering Callback Queries in Telegram
~~~~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Answering Callback Queries in Telegram
~~~~~~~~~~~~~~~~~~~~~~~~~~
Answering Callback Queries in Telegram
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite understand why amount of ~ characters here should be more than in Updating Telegram Messages title case above

Copy link
Contributor

Choose a reason for hiding this comment

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

For RST it just needs a minimum of three characters (IIRC), but in the Symfony docs we make as long as the title itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, ok. Somehow I did not get it. Thank you, updated the file

Comment on lines 352 to 354
$callbackQueryId = '12345', // extracted from callback
$showAlert = true,
$cacheTime = 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$callbackQueryId = '12345', // extracted from callback
$showAlert = true,
$cacheTime = 1,
callbackQueryId: '12345', // extracted from callback
showAlert: true,
cacheTime: 1,

WDYT of using named arguments here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I like it, thank you

@OskarStark OskarStark force-pushed the notifier-telegram-answer-callback-query branch from 4139340 to c22c47b Compare December 5, 2022 21:36
@OskarStark
Copy link
Contributor

Thanks Alex for working on this feature, this is much appreciated.

@OskarStark OskarStark merged commit b59499e into symfony:6.3 Dec 5, 2022
@alexsoft
Copy link
Contributor Author

alexsoft commented Dec 5, 2022

Thank you very much, Oskar, for reviewing and helping with PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants