Skip to content

Conversation

@wardetu
Copy link
Contributor

@wardetu wardetu commented Jul 2, 2019

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.

Copy link
Contributor

@geshuming geshuming left a comment

Choose a reason for hiding this comment

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

Some preliminary reviews.

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
Copy link
Contributor

@geshuming geshuming left a 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?

Copy link
Contributor

@rrtheonlyone rrtheonlyone left a 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)

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

@geshuming
Copy link
Contributor

geshuming commented Jul 6, 2019

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
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.

Copy link
Contributor

@geshuming geshuming left a comment

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