Skip to content

Conversation

@timmydoza
Copy link
Contributor

@timmydoza timmydoza commented Mar 17, 2021

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):

Description

This PR adds support for sending and receiving files while in a room. It uses the Twilio Conversations SDK to send the files in the chat. I left a link to a quick-deploy version of this PR in our slack channel if you want to try it out.

On Monday when I get back I'll take a look at the failing test. Otherwise I think the code is ready for review.

files

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 timmydoza changed the base branch from master to feature/conversations March 17, 2021 20:25
@timmydoza timmydoza requested a review from charliesantos March 17, 2021 23:49
<SendMessageIcon />
</button>
<Grid container alignItems="flex-end" justify="flex-end" wrap="nowrap">
<input ref={fileInputRef} type="file" style={{ display: 'none' }} onChange={handleSendFile} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we filter the files here to only allow what Twilio Conversations support? https://www.twilio.com/docs/conversations/conversations-limits#media-limits
I believe you can use the accept attribute. Also, add check for max file size. If it's more than what Twilio Conversations support, don't even start uploading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think those limits are only relevant to Conversations that send messages to an SMS channel. In Conversations that only have chat participants, any file type can be sent. All file types that I have uploaded in chat have succeeded.

Also, the Conversations SDK already checks the file size and lets us know when it is too large, in which case we display a useful error to the user:

image

Is there a benefit to checking the file size ourselves instead of letting the SDK do it?

Copy link
Collaborator

@charliesantos charliesantos Mar 25, 2021

Choose a reason for hiding this comment

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

If the SDK is already checking the file size, then I think we're good. How about the file types? Is there a doc somewhere that mentions unsupported/supported files? For example, it's generally not advisable to upload executable files. I'm curious how the SDK or server handles this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like there are no limits with respect to the file types that can be sent between two chat participants. I've even been able to send executable files in my tests.

So any file type limitations we add would be specific to our app only, and not specific to the Conversations platform. I'm happy to add some (it's technically easy), but I think this is a decision that Sean or Donal should weigh in on. I'll ask around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charliesantos I added the accept attribute to the file input. I used some MIME types instead of file extensions just to allow for some greater flexibility. Also, using image/* allows mobile users to take a picture to upload to the chat. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks great! Thanks for adding this. 🚢 🚀

@timmydoza timmydoza requested a review from charliesantos March 26, 2021 22:16
@timmydoza timmydoza merged commit 82e4928 into feature/conversations Mar 26, 2021
@timmydoza timmydoza deleted the VIDEO-3888-file-input-for-conversations branch March 26, 2021 22:41
timmydoza pushed a commit that referenced this pull request Mar 31, 2021
* VIDEO 3731 Conversations UI - create chat window (#421)

* Video 4078 add flip camera button to mobile menu (#433)

VIDEO 4078 Conversations - Add flip camera button to 'more' mobile menu

* Video 3730 add conversation hooks (#438)

* Add initial hooks for chat

* Update hooks in chat provider

* Add tests for conversationProvider component

* Fix DeviceSelectionScreen tests

* Update variable names

* Revert proxy url in package.json

* Fix linting issues

* Make error handling more user friendly

* Fix a test in src/state

* Temporarily disable cypress tests

* Undo prettier changes to config.yml

* Update chatProvider test

* Cleanup ChatProvider

* Video 3732 Conversations UI - create message components (#441)

* Add links to TextMessage component (#446)

* Add links to TextMessage component

* Update CSS in TextMessage

* Update snapshot test

* Fix issue in addLinks function

* Use forEach loop in addLinks function

* Add new server (#443)

* Use new server with reused code

* Add firebase auth to server

* Convert server to typescript

* Add tests for firebaseAuthMiddleware

* Refactor server

* Add more server tests

* Add tests for CreateExpressHandler

* Fix linter issue

* Delete old server

* Fix linter issues

* Fix some server issues

* Set up server build steps

* Change request to match new server api

* Fix token fetch test

* Add server build step to circleci config

* Enable cyress tests in CI

* Undo prettier changes in config.yml

* install plugin-rtc from npm

* Fix firebaseAuth test

* Add create_conversation:true to token requests

* Upgrade plugin-rtc to 0.8.0

* Temorarily remove firebaseAuthMiddleware and recording rules route

* Add ChatProvider back in

* Update .env.example

* Change build:server script

* Add firebaseAuthMiddleware

* Add comments to firebaseAuthMiddleware

* Update useFirebaseAuth tests

* Video 3728 Conversations UI - add input field and send button (#451)

* Chat notification (#454)

* Add notification to chat icon

* Add tests for ToggleChatButton

* Some cleanup

* Fix linting issue

* Video-4525 loading screen (#461)

* Video 4454 Conversations UI -  message list scrolling (#467)

* Update MessageList component to change how messages are grouped (#469)

* Update MessageList component to change how messages are grouped

* Simplify MessageList code

* Video 3888 file input for conversations (#458)

* Add FileAttachmentIcon

* Add File Input to ChatInput component

* Add change handler to file input

* Create MediaMessage component and add it to MessageList

* Toggle ChatWindow visibility with CSS

* Update messageList tests

* Add function to format the file size

* Update error handling in ChatInput component

* Add tests for MediaMessage component

* Update snapshot test

* Add tests for ChatInput

* Fix linting issue

* Update filesize formatting

* Update error language and tests

* Reset file input value after file sent

* Center icon in MediaMessage

* Update Icon size

* Fix breaking ChatInput test

* Update snapshot

* Use accept attribute on file input element

* Update readme to add information about the chat feature and conversations api

* Cross browser fixes (#472)

* Update app.yaml for the new server

* Add cache controls for static assets

* Only hide Screenshare button on mobile devices

* Auto focus on chat input when user opens chat window

* Fix iOS bug with media messages

* Fix issues in MessageListScrollContainer

* Fix app.yaml

* Fix infinite loop when there is a conversations error

* Update tests for ChatInput

* Update mediamessage test

* Update menubar tests

* Update messageList snapshot

* Fix linting issue

* Add comments to the server

* Video 3727 - cypress testing (#473)

* Video 4597 feature flag (#475)

* Update plugin-rtc version

* Add feature flag for Twilio Conversations

* Add tests

* Add info to .env.example

* Update readme with info about twilio conversations (#476)

* Update readme with info about twilio conversations

* Update readme to address feedback

* Release 0.3.0 (#479)

* Bump version and update changelog

* Add plugin-rtc upgrade instructions to the readme

Co-authored-by: olipyskoty <77076398+olipyskoty@users.noreply.github.com>
Co-authored-by: Sean Coleman <smcoleman27@gmail.com>
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.

3 participants