-
Notifications
You must be signed in to change notification settings - Fork 174
Adding chat component #648
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
Conversation
…tend into token_provider
Won't render the text area and bottom until ChatManager is connected. After it is connected, losing connection does not result in crash when Send is hit.
geshuming
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some preliminary reviews.
The line is to demarcate message list and input area since their color is the same now
geshuming
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second round of comments.
I don't think we should reuse the comment field. @rrtheonlyone do you think we should migrate the changes properly? I think we can just deprecate comment, but should we also remove it?
`npm run win-start` script was removed for some reason. `npm start` suffices anyway.
rrtheonlyone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good to me 👍 The scrolling works and the chat does seem to working. However, lets not merge this till the backend PR is merged (source-academy/backend#406).
A minor gripe is that it is a little slow to load. (however, that cant be helped)
This is probably because the api call is made when the chat component is mounted (i.e. when you open the chat tab) A suggestion is to move the api calls to the parent component (Side Content Component), and pass down the results to the chat component |
|
I think its better to have it isolated for now! Let us not couple it with the parent just yet (till we are sure this is the route we want to take in the long term). The API calls are done by the The app is still usable for an end-user and I feel its not a major hindrance. |
geshuming
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good

Integrated Chatkit to serve chats that will replace the current comment field in all assignments. Chatkit's documentation can be found here. For Chatkit to work, it must be used with backend.
What the chat looks like:

Changes made:
Chatincomponents, where all Chatkit related files are storedChatContainer.tsincontainers_chat.scssinstyles.env-sampleto include option to disable Chatkit.jwt-decodelibrary for chat JWT decoding.Todo:
ChatApp.jsin TypeScript once this PR is mergedNew issues will be created for the items on the todo list.