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

Sample: Password input activity #1569

Merged
merged 19 commits into from
Jan 18, 2019

Conversation

compulim
Copy link
Contributor

@compulim compulim commented Jan 10, 2019

Fix #1547 and fix #1413

Background

This sample will add a custom activity that show a password input and the content will be submitted thru postback.

image

The custom activity looks like:

{
  "name": "passwordInput",
  "type": "event"
}

Design considerations

Today, we are hiding activities thru a predicate function named shouldShowActivity in BasicTranscript.js. Although this approach make timestamp grouping logic straightforward, the user has no control on what type of activities will be visible. Currently, we only show message activity and bot's typing activity.

We are moving hide/show logic to activity middleware instead. So all activity middleware will be able to process all types of activity and decide which should be show or hidden.

For our default activity middleware, we will show message and bot's typing activity, and hide empty message, event, postback and user's typing. For all other unhandled activity type, they will show up as an error box.

Since custom middleware are always executed before default middleware, any new activity type should be handled by custom middleware. Middleware can choose to override visibility. For example, in this sample, the middleware has chosen to show event activity as a password input box, before sinking the activity to the default middleware, which will hide the event activity.

For this change, we are forced to group timestamp only after render complete (after middleware finish). Thus, we will need to use CSS to filter out timestamp on render time, instead of JavaScript logic as of today.

Changelog

Fixed

  • component: Fix #1547. Fixed unhandled activity type should be forwarded to custom middleware, by @compulim in PR #1569

Samples

@compulim compulim added the Sample Implement PoC or sample code label Jan 10, 2019
@coveralls
Copy link

coveralls commented Jan 10, 2019

Pull Request Test Coverage Report for Build 781

  • 23 of 27 (85.19%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 47.906%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/component/src/BasicTranscript.js 12 13 92.31%
packages/component/src/Middleware/Activity/createCoreMiddleware.js 10 13 76.92%
Files with Coverage Reduction New Missed Lines %
packages/component/src/Utils/Timer.js 1 33.33%
Totals Coverage Status
Change from base Build 765: -0.04%
Covered Lines: 784
Relevant Lines: 1475

💛 - Coveralls

@compulim compulim changed the title [DRAFT] Sample: Password input activity Sample: Password input activity Jan 10, 2019
Copy link
Contributor

@corinagum corinagum left a comment

Choose a reason for hiding this comment

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

Nitpicky things

README.md Outdated Show resolved Hide resolved
samples/10.b.customization-password-input/README.md Outdated Show resolved Hide resolved
samples/10.b.customization-password-input/index.html Outdated Show resolved Hide resolved
samples/10.b.customization-password-input/index.html Outdated Show resolved Hide resolved
samples/10.b.customization-password-input/index.html Outdated Show resolved Hide resolved
samples/10.b.customization-password-input/index.html Outdated Show resolved Hide resolved
samples/10.b.customization-password-input/index.html Outdated Show resolved Hide resolved
@compulim
Copy link
Contributor Author

@corinagum all comments fixed. 😄

@cwhitten
Copy link
Member

What is the plan to address the coverage?

@compulim
Copy link
Contributor Author

compulim commented Jan 15, 2019

@cwhitten Post 4.3, we will start writing tests. And in this mode, we will reject any PRs with decreased coverage. I don't know how far we could go if we strictly enforce it, but I suggest we should try this strategy out first and see how far we can stretch.

For tests that is not visual (no UI), for example, speech, I already have a plan on how to make this feature visible for our visual regression test engine (hint: via middleware and ponyfill).

For now, with only 1 test (Corina is adding some), the coverage number usually fluctuate +/- 2%. IMO, this is acceptable before we start writing comprehensive tests.

@compulim
Copy link
Contributor Author

@corinagum can you review this again for me? I made some change to the timestamp grouping to fix test failures.

Copy link
Contributor

@corinagum corinagum left a comment

Choose a reason for hiding this comment

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

Minor comments & one question :)

__tests__/constants.json Outdated Show resolved Hide resolved
packages/component/src/BasicTranscript.js Outdated Show resolved Hide resolved
packages/component/src/BasicTranscript.js Outdated Show resolved Hide resolved
@compulim compulim merged commit 42e46ad into microsoft:master Jan 18, 2019
@compulim compulim deleted the sample-password-input branch January 18, 2019 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sample Implement PoC or sample code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React component not forwarding all activities to the activityMiddleware Sample: password input activity
4 participants