search

imranweb / slack-clone

0 stars 0 forks source link

Review Comments #1

Closed Fawke closed 6 years ago

Fawke commented 6 years ago
  1. Unless we send a message on a channel, its shows loading progress indicator. [BUG]
  2. Separate out services in Sagas file. [Mandatory]
  3. Add users to a private room. [Mandatory]
  4. Multi-tenancy/multiple-teams/onboarding
  5. Login -> Takes time, shows progress indicator to the user [BUG]
  6. Try to reduce the number of container classes by using containment/composition
  7. Configure eslint with airbnb. [Mandatory]
  8. Write unit test cases for React components. [Mandatory]
imranweb commented 6 years ago

Please find the updated status:

  1. Unless we send a message on a channel, its shows loading progress indicator. [DONE]
  2. Separate out services in Sagas file. [DONE]
  3. Add users to a private room. [Mandatory]
  4. Multi-tenancy/multiple-teams/onboarding
  5. Login -> Takes time, shows progress indicator to the user [DONE]
  6. Try to reduce the number of container classes by using containment/composition (Any example?)
  7. Configure eslint with airbnb. [DONE]
  8. Write unit test cases for React components. [Written for Reducer but, Enzyme is not working as expected]
Fawke commented 6 years ago

Try to reduce the number of container classes by using containment/composition (Any example?) Refer to this article: https://reactjs.org/docs/composition-vs-inheritance.html

Let's say, you've got a component hierarchy like this, 

<P>
  <C>
    <C1>
    <C1>
  <C/>
<P/>

you need to pass props from <P> component to <C1/> component, there are 2 ways to solve this problem, one way is to use connect and create container component for them, the other way is to use composition, then your structure would look something like this.

<P>
  <C>
  <C/>
  <C1/>
  <C1>
<P/>

althrough, there is nothing wrong in your approach, but we should know all different ways to solve a problem.

  1. Write unit test cases for React components. [Written for Reducer but, Enzyme is not working as expected]

can you show me a screenshot of the error please?

imranweb commented 6 years ago

Please find the updates:

  1. Add users to a private room. [DONE]
  2. Try to reduce the number of container classes by using containment/composition (Thanks for explanation above, I will try to implement this and update you once it is done)
  3. Write unit test cases for React components. [will attached the screenshot in a while]
Fawke commented 6 years ago

Hi, you just need to close point 2 and 3, post that, you'll be marked as complete.

Fawke commented 6 years ago

Hi Imran, please write unit test cases for all the React components. Your deployed app on https://slack-chatkit.herokuapp.com/ is throwing an error.

Fawke commented 6 years ago

This project is being marked as completed.