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

On-boarding tutorial #231

Merged
merged 28 commits into from
Nov 4, 2020
Merged

On-boarding tutorial #231

merged 28 commits into from
Nov 4, 2020

Conversation

nonumpa
Copy link
Member

@nonumpa nonumpa commented Oct 27, 2020

Fixes #216

Text setup in rich menu to trigger tutorial should be:
📖 tutorial (en)
📖 教學 (zh-tw)

Snapshot

@coveralls
Copy link

coveralls commented Oct 27, 2020

Pull Request Test Coverage Report for Build 1199

  • 79 of 79 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.0%) to 85.94%

Totals Coverage Status
Change from base Build 1189: 1.0%
Covered Lines: 780
Relevant Lines: 896

💛 - Coveralls

Copy link
Member

@MrOrz MrOrz left a comment

Choose a reason for hiding this comment

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

I have not finish reviewing handlers/tutorial.js. Here are some early feedback.

src/webhook/handlers/tutorial.js Outdated Show resolved Hide resolved
src/webhook/handlers/utils.js Outdated Show resolved Hide resolved
src/webhook/handlers/tutorial.js Outdated Show resolved Hide resolved
src/webhook/handlers/tutorial.js Outdated Show resolved Hide resolved
@nonumpa nonumpa marked this pull request as ready for review October 28, 2020 11:10
@MrOrz MrOrz temporarily deployed to rumors-line-bot-staging October 28, 2020 12:46 Inactive
@MrOrz MrOrz temporarily deployed to rumors-line-bot-staging October 28, 2020 13:17 Inactive
@nonumpa nonumpa force-pushed the feature/onboarding-tutorial branch 2 times, most recently from da1203f to aaca9e0 Compare October 29, 2020 11:13
Base automatically changed from fix/liff-setting-ux to dev October 29, 2020 15:16
@nonumpa nonumpa force-pushed the feature/onboarding-tutorial branch 2 times, most recently from e78fd9b to fec5de9 Compare October 31, 2020 08:48
Copy link
Member

@MrOrz MrOrz left a comment

Choose a reason for hiding this comment

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

The unit tests are extremely helpful when I read through the code. Thanks for providing the thorough tests cases!

I have some comments regarding wording and how replies array can be constructed.

src/webhook/handlers/tutorial.js Outdated Show resolved Hide resolved
src/webhook/handlers/tutorial.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@nonumpa nonumpa force-pushed the feature/onboarding-tutorial branch from fec5de9 to 6132c7f Compare November 4, 2020 06:49
@nonumpa nonumpa force-pushed the feature/onboarding-tutorial branch from 6132c7f to b1989a9 Compare November 4, 2020 06:51
In order to achieve that, I need to ask for your permission to \\"send your message to our chatroom\\".
The permission will be used to send only this one message of yours back to this parti⋯⋯
In order to achieve that, I need to ask for your permission to \\"Send messages to chats\\".
The permission will be used to send only this one message of yours back to this particular chatr⋯⋯
Copy link
Member Author

Choose a reason for hiding this comment

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

Text will ellipsis before assigning to altText, so the snapshot looks a little weird.

Copy link
Member

@MrOrz MrOrz left a comment

Choose a reason for hiding this comment

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

LGTM! Let's merge!

@MrOrz MrOrz merged commit 0917291 into dev Nov 4, 2020
@MrOrz MrOrz deleted the feature/onboarding-tutorial branch November 4, 2020 11:33
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.

On-boarding tutorial
3 participants