Video 3728 add input field and send button

[olipyskoty]

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

• I acknowledge that all my contributions will be made under the project's license.

Pull Request Details

JIRA link(s):

• VIDEO-3728

Description

This PR adds an input field and send button to the chat window so the user can type and send messages to other participants. The send button will be highlighted in blue when anything other than whitespace is typed. Users can press enter or click the button to send a message, while enter + shift creates a new line. Please note that actually sending a message in this PR will throw an error until the new server is complete and merged. We also decided to keep the blue outline around the <TextareaAutosize /> when it is focused for accessibility reasons.

web:

mobile:

Burndown

Before review

• Updated CHANGELOG.md if necessary
• Added unit tests if necessary
• Updated affected documentation
• Verified locally with npm test
• Manually sanity tested running locally
• Included screenshot as PR comment (if needed)
• Ready for review

Before merge

• Got one or more +1s
• Re-tested if necessary

[timmydoza]

Here we can simply pass the functions themselves:

Suggested change

	onKeyPress={isMobile ? () => null : event => handleReturnKeyPress(event)}
	onChange={event => handleChange(event)}
	onKeyPress={handleReturnKeyPress}
	onChange={handleChange}

I think the isMobile check can be up on line 54. See my other comment.

[timmydoza]

We can simplify line 76 by including the isMobile check here:

Suggested change

	if (event.key === 'Enter' && !event.shiftKey) {
	if (!isMobile && event.key === 'Enter' && !event.shiftKey) {

[timmydoza]

Let's call this ChatInput to be consistent with the filename:

Suggested change

	interface InputFieldProps {
	conversation: Conversation;
	}
	export default function InputField({ conversation }: InputFieldProps) {
	interface ChatInputProps {
	conversation: Conversation;
	}
	export default function ChatInput({ conversation }: InputFieldProps) {

[olipyskoty]

ahh good catch - missed this one 👍

[timmydoza]

All these tests look good! But I think we could use some additional tests for the handleReturnKeyPress function. We'll need to test that it works correctly for the different values of isMobile, event.key, and event.shiftKey.

CircleCI provides a test coverage report for each PR (which can also be generated locally with npm run test:ci), which can be used to see if there are any tests that are missing: https://1225-216070925-gh.circle-artifacts.com/0/coverage/lcov-report/src/components/ChatWindow/ChatInput/ChatInput.tsx.html

[timmydoza]

Even better is:

Suggested change

	expect(mockHandleSendMessage).toHaveBeenCalled();
	expect(mockHandleSendMessage).toHaveBeenCalledWith('I am a message!!!');

Also, we could have a test to make sure that the whitespace is being trimmed. We could probably just use this same test for that:

wrapper.find(TextareaAutosize).simulate('change', { target: { value: ' I am a message!!! \n ' } });

[timmydoza]

I used this for my screenshots in slack:

padding: '0.75em 1em`,
marginTop: `0.4em`

But feel free to play with it. padding: '0.75em 1em makes the padding identical to the padding on the text inputs you see before you enter a room.

[olipyskoty]

i think this looks good and i like how it stays consistent with the inputs on the "login" page

[olipyskoty]

@timmydoza updating the <div> to <button> tag in the <ChatWindowHeader> now includes the blue outline when tabbing to the <button>, or when clicking on it. just fyi - but good for accessibility!

[timmydoza]

Nit: I think adding padding: '0.4em' helps a little. It makes the blue border square when it is focused. Previously it was a little rectangular.

[timmydoza]

Looks good! But now that this is a button, I think we should use the disabled prop when the button isn't supposed to be clickable:

      <button className={classes.sendButton} onClick={() => handleSendMessage(messageBody)} disabled={!isValidMessage}>
        <SendMessageIcon />
      </button>

When disabled is true, it tells the browser that the button isn't supposed to be clickable. As a result, the button won't be tabbable when it's disabled. So I think it goes a little further than just changing the styles so that it doesn't look clickable.

Also, with this change, we can take advantage of the :disabled pseudo-class to apply the right styles when it's disabled. This way we don't have to worry about conditionally applying the styles ourselves. So the sendButton class can look like:

  sendButton: {
    height: '40px',
    width: '40px',
    border: '0',
    borderRadius: '4px',
    float: 'right',
    display: 'flex',
    alignItems: 'center',
    justifyContent: 'center',
    marginTop: '1em',
    background: '#0263E0',
    cursor: 'pointer',
    '&:disabled': {
      background: 'none',
      cursor: 'default',
    },
  },

The & is just a substitute for the classname of the parent object. So '&:disabled' gets turned into .sendButton:disabled.

Then, the test will have to be updated to look for the disabled prop instead of the activeSendButton class (which we can get rid of). For example:

    expect(
      wrapper
        .find(SendMessageIcon)
        .parent()
        .prop('disabled')
    ).toBe(true);

[olipyskoty]

good point! this looks nicer too