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

Happychat: Middleware refactor of HAPPYCHAT_SET_MESSAGE #12565

Merged

Conversation

mattwondra
Copy link
Member

@mattwondra mattwondra commented Mar 27, 2017

Moves side effects related to HAPPYCHAT_SET_MESSAGE actions into the middleware. Part of an ongoing refactor.

HAPPYCHAT_SET_MESSAGE updates the controlled form input state in the Happychat composer when a user types. The side effect is that the remote operator is informed when the user starts and stops typing.

To test

  • In side-by-side windows pull up the Happychat HUD, http://calypso.localhost:3000/me/chat, and Redux Dev Tools inspecting the Calypso instance
  • In Redux Dev Tools, open the happychat subtree. The message key should initially have a value ''
  • Start typing in the message box in Calypso
    • Your characters should appear in the Calypso text box
    • Redux happychat.message should be syncing in real-time with your changes
    • The HUD should show the user typing as soon as you start
  • Start typing and then delete everything you typed
    • The HUD should show the user as typing right up until the message box is blank, when the typing animation should disappear completely

@matticbot
Copy link
Contributor

@mattwondra mattwondra added Happychat [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 27, 2017
@mattwondra mattwondra self-assigned this Mar 27, 2017
Alphabetize!

Alphabetize... again

Update middleware helper function to have better name
@mattwondra mattwondra force-pushed the update/happychat/middleware-refactor/set-chat-message branch from 4a55987 to 2217fe1 Compare March 27, 2017 20:13
Copy link
Contributor

@southp southp left a comment

Choose a reason for hiding this comment

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

Tested the test plan plus npm run test-client client/state/happychat/test, and everything worked as expected. Great job!

I've left one comment, but it was soft and shouldn't hold you back 🚢

it( 'should send the connection a typing signal when a message is present', () => {
const action = { type: HAPPYCHAT_SET_MESSAGE, message: 'Hello world' };
const connection = { typing: spy() };
middleware( connection )()( spy() )( action );
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are not really interested in what's happening to the next function here, could we consider to use noop from lodash instead of creating a spy instance here? It would be a little bit light weighted and the "don't care" intention would be clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great suggestion 👍

it( 'should send the connection a notTyping signal when the message is blank', () => {
const action = { type: HAPPYCHAT_SET_MESSAGE, message: '' };
const connection = { notTyping: spy() };
middleware( connection )()( spy() )( action );
Copy link
Contributor

Choose a reason for hiding this comment

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

The same as the above.

@mattwondra mattwondra added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 29, 2017
@mattwondra mattwondra merged commit 425411d into master Mar 29, 2017
@mattwondra mattwondra deleted the update/happychat/middleware-refactor/set-chat-message branch March 29, 2017 16:16
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