Skip to content

Adding chat component#648

Merged
rrtheonlyone merged 61 commits into
source-academy:masterfrom
wardetu:master
Jul 7, 2019
Merged

Adding chat component#648
rrtheonlyone merged 61 commits into
source-academy:masterfrom
wardetu:master

Conversation

@wardetu

@wardetu wardetu commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

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

Changes made:

  1. New folder Chat in components, where all Chatkit related files are stored
  2. New file ChatContainer.ts in containers
  3. New file _chat.scss in styles
  4. Edited .env-sample to include option to disable Chatkit.
  5. Add jwt-decode library for chat JWT decoding.

Todo:

  1. Rewrite ChatApp.js in TypeScript once this PR is merged

New issues will be created for the items on the todo list.

@geshuming geshuming left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some preliminary reviews.

Comment thread package.json Outdated
Comment thread package.json Outdated
Comment thread src/components/chat/ChatApp.js Outdated
Comment thread src/components/chat/Input.tsx Outdated
wardetu added 4 commits July 3, 2019 17:11
The line is to demarcate message list and input area since their color is the same now

@geshuming geshuming left a comment

Copy link
Copy Markdown
Contributor

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?

Comment thread src/components/academy/grading/GradingWorkspace.tsx
Comment thread src/components/assessment/AssessmentWorkspace.tsx Outdated
Comment thread src/components/assessment/AssessmentWorkspace.tsx Outdated
Comment thread package.json Outdated

@rrtheonlyone rrtheonlyone left a comment

Copy link
Copy Markdown
Contributor

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)

Here is a (blurred) gif how it feels:
test_chat

@geshuming

geshuming commented Jul 6, 2019

Copy link
Copy Markdown
Contributor

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

@rrtheonlyone

Copy link
Copy Markdown
Contributor

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 chatManager and it may be difficult to shift it out to the parent.

The app is still usable for an end-user and I feel its not a major hindrance.

Comment thread src/components/chat/ChatApp.js Outdated

@geshuming geshuming left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good

@rrtheonlyone rrtheonlyone merged commit 7bba082 into source-academy:master Jul 7, 2019
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.

6 participants