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.

Sign up for GitHub

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

Jump to bottom

Update readme with info about twilio conversations #476

Merged
merged 2 commits into from
Mar 31, 2021
Merged

Update readme with info about twilio conversations #476

merged 2 commits into from
Mar 31, 2021

Conversation

timmydoza
Copy link
Contributor

@timmydoza timmydoza commented Mar 31, 2021 •
edited
Loading

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

  • N/A

Description

This PR adds some additional context to the README about the new chat feature. You can see the formatted README here: https://github.com/twilio/twilio-video-app-react/tree/conversations-documentation-updates

(apologies about all the changes. I forgot to disable my automatic formatter for this file.)

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

seancoleman2
seancoleman2 reviewed Mar 31, 2021
View reviewed changes
README.md Outdated
@@ -131,7 +131,9 @@ TWILIO_API_KEY_SECRET=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
TWILIO_CONVERSATIONS_SERVICE_SID=ISxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
```

Now the local token server (see [server.js](server.js)) can dispense Access Tokens to connect to a Room and a Conversation.
Now the local token server (see [server.js](server/index.ts)) can dispense Access Tokens to connect to a Room and a Conversation. See [.env.example](.env.example) for information on additional environment variables that can be used.
Copy link
Contributor

Choose a reason for hiding this comment

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

server.js is no longer? --> (see server/index.ts)

olipyskoty
olipyskoty approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@olipyskoty olipyskoty left a comment

Choose a reason for hiding this comment

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

This looks good to me and reads well. One small typo that I found and a suggestion on drawing more attention to the bits about the conversations feature being optional/being charged while testing.

README.md Outdated
@@ -116,12 +116,12 @@ See browser support table for [twilio-video.js SDK](https://github.com/twilio/tw

### Running a local token server

This application requires an access token to connect to a Room for Video and a Conversation for Chat. The included local token [server](server.js) provides the application with access tokens. Perform the following steps to setup the local token server:
This application requires an access token to connect to a Room for Video and a Conversation for Chat. The included local token [server](server/index.ts) provides the application with access tokens. This token server can be used to run the app localally, and it is the server that is used when this app is run in development mode with `npm start`. Perform the following steps to setup the local token server:
Copy link
Contributor

Choose a reason for hiding this comment

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

small typo - run the app localally. Should be locally

README.md Outdated
Now the local token server (see [server.js](server.js)) can dispense Access Tokens to connect to a Room and a Conversation.
Now the local token server (see [server.js](server/index.ts)) can dispense Access Tokens to connect to a Room and a Conversation. See [.env.example](.env.example) for information on additional environment variables that can be used.

Note: the use of Twilio Conversations is optional. If you wish to opt out, simply run or build this app with the `REACT_APP_DISABLE_TWILIO_CONVERSATIONS` environment variable set to `true`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if "Note" should be bolded so that we can draw more attention to this line so that customers know that it's optional?

README.md Outdated
@@ -193,11 +193,11 @@ Run end to end tests with

This will open the Cypress test runner. When it's open, select a test file to run.

Note: Be sure to complete the 'Getting Started' section before running these tests. These Cypress tests will connect to real Twilio rooms, so you may be billed for any time that is used.
Note: Be sure to complete the 'Getting Started' section before running these tests. These Cypress tests will connect to real Twilio rooms and real Twilio conversations, so you may be billed for any time that is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here? Maybe make "Note" bolded?

charliesantos
charliesantos reviewed Mar 31, 2021
View reviewed changes
Copy link
Collaborator

@charliesantos charliesantos left a comment

Choose a reason for hiding this comment

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

I'll skip reviewing this and let Sean and Olivia look at it.

seancoleman2
seancoleman2 approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@seancoleman2 seancoleman2 left a comment

Choose a reason for hiding this comment

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

LGTM

@timmydoza timmydoza merged commit 4ebacc2 into feature/conversations Mar 31, 2021
@timmydoza timmydoza deleted the conversations-documentation-updates branch March 31, 2021 16:44
timmydoza pushed a commit that referenced this pull request Mar 31, 2021
@olipyskoty @seancoleman2
Feature/conversations (#480)
b12ba32 
* 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
Reviewers

@charliesantos charliesantos charliesantos left review comments

@seancoleman2 seancoleman2 seancoleman2 approved these changes

@olipyskoty olipyskoty olipyskoty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@timmydoza @seancoleman2 @charliesantos @olipyskoty