From db6831603123dfdaf763bdf2f0fae85f044b99a9 Mon Sep 17 00:00:00 2001
From: Jeremie Pardou-Piquemal <571533+jrmi@users.noreply.github.com>
Date: Wed, 13 May 2020 22:06:51 +0200
Subject: [PATCH] Rework Home component phase 2

---
 client/src/components/Home/Activity.js        | 227 ++++++++----------
 client/src/components/Home/ActivityList.js    | 136 +++++------
 .../src/components/Home/ActivityList.test.js  |  95 +++-----
 client/src/components/Home/Home.js            |  20 +-
 client/src/components/Home/index.js           |   3 -
 5 files changed, 207 insertions(+), 274 deletions(-)

diff --git a/client/src/components/Home/Activity.js b/client/src/components/Home/Activity.js
index a531c51..61cd116 100644
--- a/client/src/components/Home/Activity.js
+++ b/client/src/components/Home/Activity.js
@@ -1,4 +1,4 @@
-import React, { Component } from 'react'
+import React from 'react'
 import PropTypes from 'prop-types'
 import Message from 'components/Message'
 import Username from 'components/Username'
@@ -8,143 +8,128 @@ import { getObjectUrl } from 'utils/file'
 
 import T from 'components/T'
 
-class Activity extends Component {
-  constructor(props) {
-    super(props)
+const FileDisplay = ({ activity: { fileType, encodedFile, fileName, username }, scrollToBottom }) => {
+  const zoomableImage = React.useRef(null)
 
-    this.state = {
-      zoomableImages: [],
-    }
+  const handleImageDisplay = () => {
+    Zoom(zoomableImage.current)
+    scrollToBottom()
   }
 
-  getFileDisplay(activity) {
-    const type = activity.fileType
-    if (type.match('image.*')) {
+  if (fileType.match('image.*')) {
+    return (
+      <img
+        ref={zoomableImage}
+        className="image-transfer zoomable"
+        src={`data:${fileType};base64,${encodedFile}`}
+        alt={`${fileName} from ${username}`}
+        onLoad={handleImageDisplay}
+      />
+    )
+  }
+  return null
+}
+
+const Activity = ({ activity, scrollToBottom }) => {
+  switch (activity.type) {
+    case 'TEXT_MESSAGE':
       return (
-        <img
-          ref={c => this._zoomableImage = c}
-          className="image-transfer zoomable"
-          src={`data:${activity.fileType};base64,${activity.encodedFile}`}
-          alt={`${activity.fileName} from ${activity.username}`}
-          onLoad={this.handleImageDisplay.bind(this)}
+        <Message
+          sender={activity.username}
+          message={activity.text}
+          timestamp={activity.timestamp}
         />
       )
-    }
-    return null
-  }
-
-  handleImageDisplay() {
-    Zoom(this._zoomableImage)
-    this.props.scrollToBottom()
-  }
-
-  getActivityComponent(activity) {
-    switch (activity.type) {
-      case 'TEXT_MESSAGE':
-        return (
-          <Message
-            sender={activity.username}
-            message={activity.text}
-            timestamp={activity.timestamp}
-          />
-        )
-      case 'USER_ENTER':
-        return (
-          <Notice>
-            <div>
-              <T data={{
-                username: <Username key={0} username={activity.username} />
-              }} path='userJoined'/>
-            </div>
-          </Notice>
-        )
-      case 'USER_EXIT':
-        return (
-          <Notice>
-            <div>
-              <T data={{
-                username: <Username key={0} username={activity.username} />
-              }} path='userLeft'/>
-            </div>
-          </Notice>
-        )
-      case 'TOGGLE_LOCK_ROOM':
-        if (activity.locked) {
-          return (
-            <Notice>
-              <div><T data={{
-                username: <Username key={0} username={activity.username} />
-              }} path='lockedRoom'/></div>
-            </Notice>
-          )
-        } else {
-          return (
-            <Notice>
-              <div><T data={{
-                username: <Username key={0} username={activity.username} />
-              }} path='unlockedRoom'/></div>
-            </Notice>
-          )
-        }
-      case 'NOTICE':
-        return (
-          <Notice>
-            <div>{activity.message}</div>
-          </Notice>
-        )
-      case 'CHANGE_USERNAME':
+    case 'USER_ENTER':
+      return (
+        <Notice>
+          <div>
+            <T data={{
+              username: <Username key={0} username={activity.username} />
+            }} path='userJoined'/>
+          </div>
+        </Notice>
+      )
+    case 'USER_EXIT':
+      return (
+        <Notice>
+          <div>
+            <T data={{
+              username: <Username key={0} username={activity.username} />
+            }} path='userLeft'/>
+          </div>
+        </Notice>
+      )
+    case 'TOGGLE_LOCK_ROOM':
+      if (activity.locked) {
         return (
           <Notice>
             <div><T data={{
-              oldUsername: <Username key={0} username={activity.currentUsername} />,
-              newUsername: <Username key={1} username={activity.newUsername} />
-            }} path='nameChange'/>
-            </div>
+              username: <Username key={0} username={activity.username} />
+            }} path='lockedRoom'/></div>
           </Notice>
         )
-      case 'USER_ACTION':
+      } else {
         return (
           <Notice>
-            <div>&#42; <Username username={activity.username} /> {activity.action}</div>
+            <div><T data={{
+              username: <Username key={0} username={activity.username} />
+            }} path='unlockedRoom'/></div>
           </Notice>
         )
-      case 'RECEIVE_FILE':
-        const downloadUrl = getObjectUrl(activity.encodedFile, activity.fileType)
-        return (
+      }
+    case 'NOTICE':
+      return (
+        <Notice>
+          <div>{activity.message}</div>
+        </Notice>
+      )
+    case 'CHANGE_USERNAME':
+      return (
+        <Notice>
+          <div><T data={{
+            oldUsername: <Username key={0} username={activity.currentUsername} />,
+            newUsername: <Username key={1} username={activity.newUsername} />
+          }} path='nameChange'/>
+          </div>
+        </Notice>
+      )
+    case 'USER_ACTION':
+      return (
+        <Notice>
+          <div>&#42; <Username username={activity.username} /> {activity.action}</div>
+        </Notice>
+      )
+    case 'RECEIVE_FILE':
+      const downloadUrl = getObjectUrl(activity.encodedFile, activity.fileType)
+      return (
+        <div>
+          <T data={{
+            username: <Username key={0} username={activity.username} />,
+          }} path='userSentFile'/>&nbsp;
+
+          <a target="_blank" href={downloadUrl} rel="noopener noreferrer" download={activity.fileName} >
+            <T data={{
+              filename: activity.fileName,
+            }} path='downloadFile'/>
+          </a>
+          <FileDisplay activity={activity} scrollToBottom={scrollToBottom} />
+        </div>
+      )
+    case 'SEND_FILE':
+      const url = getObjectUrl(activity.encodedFile, activity.fileType)
+      return (
+        <Notice>
           <div>
             <T data={{
-              username: <Username key={0} username={activity.username} />,
-            }} path='userSentFile'/>&nbsp;
-
-            <a target="_blank" href={downloadUrl} rel="noopener noreferrer" download={activity.fileName}>
-              <T data={{
-                filename: activity.fileName,
-              }} path='downloadFile'/>
-            </a>
-            {this.getFileDisplay(activity)}
+              filename: <a key={0} target="_blank" href={url} rel="noopener noreferrer" download={activity.fileName}>{activity.fileName}</a>,
+            }} path='sentFile'/>&nbsp;
           </div>
-        )
-      case 'SEND_FILE':
-        const url = getObjectUrl(activity.encodedFile, activity.fileType)
-        return (
-          <Notice>
-            <div>
-              <T data={{
-                filename: <a key={0} target="_blank" href={url} rel="noopener noreferrer" download={activity.fileName}>{activity.fileName}</a>,
-              }} path='sentFile'/>&nbsp;
-            </div>
-            {this.getFileDisplay(activity)}
-          </Notice>
-        )
-      default:
-        return false
-    }
-  }
-
-  render() {
-    return (
-      this.getActivityComponent(this.props.activity)
-    )
+          <FileDisplay activity={activity} scrollToBottom={scrollToBottom} />
+        </Notice>
+      )
+    default:
+      return false
   }
 }
 
@@ -153,4 +138,4 @@ Activity.propTypes = {
   scrollToBottom: PropTypes.func.isRequired,
 }
 
-export default Activity;
+export default Activity
diff --git a/client/src/components/Home/ActivityList.js b/client/src/components/Home/ActivityList.js
index 1e4f019..59c0124 100644
--- a/client/src/components/Home/ActivityList.js
+++ b/client/src/components/Home/ActivityList.js
@@ -1,100 +1,90 @@
-import React, { Component } from 'react'
+import React from 'react'
 import PropTypes from 'prop-types'
 import ChatInput from 'components/Chat'
-import { defer } from 'lodash'
 import Activity from './Activity'
-
 import T from 'components/T'
+import { defer } from 'lodash'
 
 import styles from './styles.module.scss'
 
-class ActivityList extends Component {
-  constructor(props) {
-    super(props)
+const ActivityList = ({ activities, openModal }) => {
+  const [focusChat, setFocusChat] = React.useState(false);
+  const [scrolledToBottom, setScrolledToBottom] = React.useState(true);
+  const messageStream = React.useRef(null);
+  const activitiesList = React.useRef(null);
 
-    this.state = {
-      zoomableImages: [],
-      focusChat: false,
-    }
-  }
+  React.useEffect(() => {
+    const currentMessageStream = messageStream.current;
 
-  componentDidMount() {
-    this.bindEvents()
-  }
+    // Update scrolledToBottom state if we scroll the activity stream
+    const onScroll = () => {
+      const messageStreamHeight = messageStream.current.clientHeight
+      const activitiesListHeight = activitiesList.current.clientHeight
 
-  componentDidUpdate(prevProps) {
-    if (prevProps.activities.length < this.props.activities.length) {
-      this.scrollToBottomIfShould()
-    }
-  }
+      const bodyRect = document.body.getBoundingClientRect()
+      const elemRect = activitiesList.current.getBoundingClientRect()
+      const offset = elemRect.top - bodyRect.top
+      const activitiesListYPos = offset
 
-  onScroll() {
-    const messageStreamHeight = this.messageStream.clientHeight
-    const activitiesListHeight = this.activitiesList.clientHeight
-
-    const bodyRect = document.body.getBoundingClientRect()
-    const elemRect = this.activitiesList.getBoundingClientRect()
-    const offset = elemRect.top - bodyRect.top
-    const activitiesListYPos = offset
-
-    const scrolledToBottom = (activitiesListHeight + (activitiesListYPos - 60)) <= messageStreamHeight
-    if (scrolledToBottom) {
-      if (!this.props.scrolledToBottom) {
-        this.props.setScrolledToBottom(true)
+      const newScrolledToBottom = (activitiesListHeight + (activitiesListYPos - 60)) <= messageStreamHeight
+      if (newScrolledToBottom) {
+        if (!scrolledToBottom) {
+          setScrolledToBottom(true)
+        }
+      } else if (scrolledToBottom) {
+        setScrolledToBottom(false)
       }
-    } else if (this.props.scrolledToBottom) {
-      this.props.setScrolledToBottom(false)
     }
-  }
 
-  scrollToBottomIfShould() {
-    if (this.props.scrolledToBottom) {
-      setTimeout(() => {
-        this.messageStream.scrollTop = this.messageStream.scrollHeight
-      }, 0)
+    currentMessageStream.addEventListener('scroll', onScroll)
+    return () => {
+      // Unbind event if component unmounted
+      currentMessageStream.removeEventListener('scroll', onScroll)
     }
+  }, [scrolledToBottom])
+
+  const scrollToBottomIfShould = React.useCallback(() => {
+    if (scrolledToBottom) {
+      messageStream.current.scrollTop = messageStream.current.scrollHeight
+    }
+  }, [scrolledToBottom])
+
+  React.useEffect(() => {
+    scrollToBottomIfShould(); // Only if activities.length bigger
+  }, [scrollToBottomIfShould, activities]);
+
+  const scrollToBottom = React.useCallback(() => {
+    messageStream.current.scrollTop = messageStream.current.scrollHeight
+    setScrolledToBottom(true)
+  }, [])
+
+  const handleChatClick = () => {
+    setFocusChat(true);
+    defer(() => setFocusChat(false))
   }
 
-  scrollToBottom() {
-    this.messageStream.scrollTop = this.messageStream.scrollHeight
-    this.props.setScrolledToBottom(true)
-  }
-
-  bindEvents() {
-    this.messageStream.addEventListener('scroll', this.onScroll.bind(this))
-  }
-
-  handleChatClick() {
-    this.setState({ focusChat: true })
-    defer(() => this.setState({ focusChat: false }))
-  }
-
-  render() {
-    return (
-      <div className="main-chat">
-        <div onClick={this.handleChatClick.bind(this)} className="message-stream h-100" ref={el => this.messageStream = el} data-testid="main-div">
-          <ul className="plain" ref={el => this.activitiesList = el}>
-            <li><p className={styles.tos}><button className='btn btn-link' onClick={this.props.openModal.bind(this, 'About')}> <T path='agreement'/></button></p></li>
-            {this.props.activities.map((activity, index) => (
+  return (
+    <div className="main-chat">
+      <div onClick={handleChatClick} className="message-stream h-100" ref={messageStream} data-testid="main-div">
+        <ul className="plain" ref={activitiesList}>
+          <li><p className={styles.tos}><button className='btn btn-link' onClick={() => openModal('About')}> <T path='agreement'/></button></p></li>
+          {activities.map((activity, index) => (
             <li key={index} className={`activity-item ${activity.type}`}>
-                <Activity activity={activity} scrollToBottom={this.scrollToBottomIfShould.bind(this)} />
+              <Activity activity={activity} scrollToBottom={scrollToBottomIfShould} />
             </li>
-            ))}
-          </ul>
-          </div>
-          <div className="chat-container">
-          <ChatInput scrollToBottom={this.scrollToBottom.bind(this)} focusChat={this.state.focusChat} />
-        </div>
+          ))}
+        </ul>
       </div>
-    )
-  }
+      <div className="chat-container">
+        <ChatInput scrollToBottom={scrollToBottom} focusChat={focusChat} />
+      </div>
+    </div>
+  )
 }
 
 ActivityList.propTypes = {
   activities: PropTypes.array.isRequired,
   openModal: PropTypes.func.isRequired,
-  setScrolledToBottom: PropTypes.func.isRequired,
-  scrolledToBottom: PropTypes.bool.isRequired,
 }
 
-export default ActivityList;
+export default ActivityList
diff --git a/client/src/components/Home/ActivityList.test.js b/client/src/components/Home/ActivityList.test.js
index af1c769..ebe234c 100644
--- a/client/src/components/Home/ActivityList.test.js
+++ b/client/src/components/Home/ActivityList.test.js
@@ -1,5 +1,5 @@
 import React from 'react';
-import { render, fireEvent, waitFor } from '@testing-library/react';
+import { render, fireEvent  } from '@testing-library/react';
 import ActivityList from './ActivityList';
 import { Provider } from 'react-redux';
 import configureStore from 'store';
@@ -12,7 +12,7 @@ describe('ActivityList component', () => {
   it('should display', () => {
     const { asFragment } = render(
       <Provider store={store}>
-        <ActivityList openModal={jest.fn()} activities={[]} setScrolledToBottom={jest.fn()} scrolledToBottom />
+        <ActivityList openModal={jest.fn()} activities={[]} />
       </Provider>,
     );
 
@@ -39,7 +39,7 @@ describe('ActivityList component', () => {
     ];
     const { asFragment } = render(
       <Provider store={store}>
-        <ActivityList openModal={jest.fn()} activities={activities} setScrolledToBottom={jest.fn()} scrolledToBottom />
+        <ActivityList openModal={jest.fn()} activities={activities} />
       </Provider>,
     );
 
@@ -51,7 +51,7 @@ describe('ActivityList component', () => {
 
     const { getByText } = render(
       <Provider store={store}>
-        <ActivityList openModal={mockOpenModal} activities={[]} setScrolledToBottom={jest.fn()} scrolledToBottom />
+        <ActivityList openModal={mockOpenModal} activities={[]} />
       </Provider>,
     );
 
@@ -59,79 +59,33 @@ describe('ActivityList component', () => {
     jest.runAllTimers();
 
     expect(mockOpenModal.mock.calls[0][0]).toBe('About');
+    jest.runAllTimers();
   });
 
   it('should focus chat', () => {
     const { getByTestId } = render(
       <Provider store={store}>
-        <ActivityList openModal={jest.fn()} activities={[]} setScrolledToBottom={jest.fn()} scrolledToBottom />
+        <ActivityList openModal={jest.fn()} activities={[]} />
       </Provider>,
     );
     fireEvent.click(getByTestId('main-div'));
     jest.runAllTimers();
   });
 
-  it('should handle scroll', () => {
-    const mockSetScrollToBottom = jest.fn();
-    jest
-      .spyOn(Element.prototype, 'clientHeight', 'get')
-      .mockReturnValueOnce(400)
-      .mockReturnValueOnce(200)
-      .mockReturnValueOnce(400)
-      .mockReturnValueOnce(200);
+  it('should scroll to bottom on new message if not scrolled', () => {
+    jest.spyOn(Element.prototype, 'clientHeight', 'get').mockReturnValueOnce(400).mockReturnValueOnce(200);
 
     Element.prototype.getBoundingClientRect = jest
       .fn()
       .mockReturnValueOnce({ top: 0 })
-      .mockReturnValueOnce({ top: 60 })
-      .mockReturnValueOnce({ top: 0 })
       .mockReturnValueOnce({ top: 261 });
 
-    const { getByTestId, rerender } = render(
-      <Provider store={store}>
-        <ActivityList
-          openModal={jest.fn()}
-          activities={[]}
-          setScrolledToBottom={mockSetScrollToBottom}
-          scrolledToBottom={false}
-        />
-      </Provider>,
-    );
-    fireEvent.scroll(getByTestId('main-div'));
-
-    expect(mockSetScrollToBottom).toHaveBeenLastCalledWith(true);
-
-    rerender(
-      <Provider store={store}>
-        <ActivityList
-          openModal={jest.fn()}
-          activities={[]}
-          setScrolledToBottom={mockSetScrollToBottom}
-          scrolledToBottom={true}
-        />
-      </Provider>,
-    );
-
-    fireEvent.scroll(getByTestId('main-div'));
-
-    expect(mockSetScrollToBottom).toHaveBeenCalledTimes(2);
-    expect(mockSetScrollToBottom).toHaveBeenLastCalledWith(false);
-  });
-
-  it('should scroll to bottom on new message', () => {
-    const mockSetScrollToBottom = jest.fn();
-
     jest.spyOn(Element.prototype, 'scrollHeight', 'get').mockReturnValue(42);
     const mockScrollTop = jest.spyOn(Element.prototype, 'scrollTop', 'set');
 
-    const { rerender } = render(
+    const { rerender, getByTestId } = render(
       <Provider store={store}>
-        <ActivityList
-          openModal={jest.fn()}
-          activities={[]}
-          setScrolledToBottom={mockSetScrollToBottom}
-          scrolledToBottom={true}
-        />
+        <ActivityList openModal={jest.fn()} activities={[]} />
       </Provider>,
     );
 
@@ -147,14 +101,39 @@ describe('ActivityList component', () => {
               text: 'Hi!',
             },
           ]}
-          setScrolledToBottom={mockSetScrollToBottom}
-          scrolledToBottom={true}
         />
       </Provider>,
     );
 
     jest.runAllTimers();
 
+    expect(mockScrollTop).toHaveBeenCalledTimes(2);
     expect(mockScrollTop).toHaveBeenLastCalledWith(42);
+
+    fireEvent.scroll(getByTestId('main-div'));
+
+    rerender(
+      <Provider store={store}>
+        <ActivityList
+          openModal={jest.fn()}
+          activities={[
+            {
+              type: 'TEXT_MESSAGE',
+              username: 'alice',
+              timestamp: new Date('2020-03-14T11:01:58.135Z').valueOf(),
+              text: 'Hi!',
+            },
+            {
+              type: 'TEXT_MESSAGE',
+              username: 'alice',
+              timestamp: new Date('2020-03-14T11:01:59.135Z').valueOf(),
+              text: 'Hi! every body',
+            },
+          ]}
+        />
+      </Provider>,
+    );
+
+    expect(mockScrollTop).toHaveBeenCalledTimes(2);
   });
 });
diff --git a/client/src/components/Home/Home.js b/client/src/components/Home/Home.js
index ef2761b..a96fb65 100644
--- a/client/src/components/Home/Home.js
+++ b/client/src/components/Home/Home.js
@@ -11,7 +11,6 @@ import Settings from 'components/Settings'
 import Welcome from 'components/Welcome'
 import RoomLocked from 'components/RoomLocked'
 import { X, AlertCircle } from 'react-feather'
-import { defer } from 'lodash'
 import Tinycon from 'tinycon'
 import beepFile from 'audio/beep.mp3'
 import classNames from 'classnames'
@@ -24,13 +23,6 @@ const crypto = new Crypto()
 Modal.setAppElement('#root');
 
 class Home extends Component {
-  constructor(props) {
-    super(props)
-
-    this.state = {
-      zoomableImages: []
-    }
-  }
 
   async componentWillMount() {
     const roomId = encodeURI(this.props.match.params.roomId)
@@ -145,7 +137,6 @@ class Home extends Component {
   }
 
   bindEvents() {
-
     window.onfocus = () => {
       this.props.toggleWindowFocus(true)
     }
@@ -175,11 +166,6 @@ class Home extends Component {
     })
   }
 
-  handleChatClick() {
-    this.setState({ focusChat: true })
-    defer(() => this.setState({ focusChat: false }))
-  }
-
   render() {
     const modalOpts = this.getModal()
     return (
@@ -201,11 +187,9 @@ class Home extends Component {
             translations={this.props.translations}
           />
         </div>
-        <ActivityList 
+        <ActivityList
           openModal={this.props.openModal}
           activities={this.props.activities}
-          setScrolledToBottom={this.props.setScrolledToBottom}
-          scrolledToBottom={this.props.scrolledToBottom}
         />
         <Modal
           isOpen={Boolean(this.props.modalComponent)}
@@ -261,8 +245,6 @@ Home.propTypes = {
   modalComponent: PropTypes.string,
   openModal: PropTypes.func.isRequired,
   closeModal: PropTypes.func.isRequired,
-  setScrolledToBottom: PropTypes.func.isRequired,
-  scrolledToBottom: PropTypes.bool.isRequired,
   iAmOwner: PropTypes.bool.isRequired,
   userId: PropTypes.string.isRequired,
   toggleWindowFocus: PropTypes.func.isRequired,
diff --git a/client/src/components/Home/index.js b/client/src/components/Home/index.js
index b90450c..ae7357f 100644
--- a/client/src/components/Home/index.js
+++ b/client/src/components/Home/index.js
@@ -5,7 +5,6 @@ import {
   createUser,
   openModal,
   closeModal,
-  setScrolledToBottom,
   toggleWindowFocus,
   toggleSoundEnabled,
   toggleSocketConnected,
@@ -28,7 +27,6 @@ const mapStateToProps = (state) => {
     roomId: state.room.id,
     roomLocked: state.room.isLocked,
     modalComponent: state.app.modalComponent,
-    scrolledToBottom: state.app.scrolledToBottom,
     iAmOwner: Boolean(me && me.isOwner),
     faviconCount: state.app.unreadMessageCount,
     soundIsEnabled: state.app.soundIsEnabled,
@@ -43,7 +41,6 @@ const mapDispatchToProps = {
   createUser,
   openModal,
   closeModal,
-  setScrolledToBottom,
   toggleWindowFocus,
   toggleSoundEnabled,
   toggleSocketConnected,