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

Add support for Text quick replies #75

Merged
merged 11 commits into from
Oct 19, 2017
Merged

Add support for Text quick replies #75

merged 11 commits into from
Oct 19, 2017

Conversation

crccheck
Copy link
Contributor

Why are we doing this?

We made a way to do Image + quick replies but not Text + quick replies. This adds that missing message type, it also deprecates the old ImageQuickReply helper in favor of plain Text and Image + .quickReplies() method. Adding quick replies isn't really a new message type, it's a modification of existing types. The syntax from the tests and README look like:

const text = new Text('Where are you?').quickReplies([{ content_type: 'location' }])

closes #60

Did you document your work?

README is updated. I wonder if this should move to the wiki though, the README is getting heavy.

How can someone test these changes?

Steps to manually verify the change:

  1. npm i
  2. npm t

What possible risks or adverse effects are there?

  • none

tech debt increases slightly but can improve when ImageQuickReply is removed. There's also some potential for a DRY refactor

What are the follow-up tasks?

  • release minor version bump

Are there any known issues?

none

Did the test coverage decrease?

new code is covered, increasing coverage

@crccheck crccheck requested a review from stripethree October 18, 2017 22:39
Copy link
Contributor

@stripethree stripethree left a comment

Choose a reason for hiding this comment

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

Looking good. 👍 Tests pass, no questions on the code as it looks straightforward.

README is updated. I wonder if this should move to the wiki though, the README is getting heavy.

Perhaps it is time for s split? Maybe theREADME can keep the project description & prior art and the wiki has everything else?

@crccheck
Copy link
Contributor Author

I may just start writing more in the wiki then when it meets a certain quality and dupes the README, delete the README version

@crccheck crccheck merged commit 5786285 into master Oct 19, 2017
@crccheck crccheck deleted the quick-reply-chains/#60 branch October 19, 2017 22:49
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.

Add support for Text quick replies
2 participants