TypeScript migration #163

mcallegari10 · 2020-04-02T18:19:47Z

Description

Full migration to TypeScript

bumped redux and react-redux versions to latest
migrate all the components to typescript
change connect react-redux hoc for hooks
add missing type for svgs to compile

- fix type issues in the redux store - bumped redux adn react-redux versions to latest

- change connect react-redux hoc for hooks - fix some store types issues

damfinkel · 2020-04-02T18:45:21Z

src/components/Widget/components/Conversation/components/Header/index.tsx

@@ -0,0 +1,32 @@
+import React from 'react';
+
+const close = require('../../../../../../../assets/clear-button.svg') as string;


why require?

TypeScript needs it to be imported that way, or at least that's the simplest way I found

damfinkel · 2020-04-02T18:46:55Z

src/components/Widget/components/Conversation/components/Messages/components/Loader/index.tsx

+
+function Loader({ typing }: Props) {
+  return (
+  <div className={`loader ${typing && 'active'}`}>


if this is false you'll have a 'false' as a class I think. You'll need to use a ternary: loader ${typing ? 'active' : ''}

Or use https://github.com/JedWatson/classnames

damfinkel · 2020-04-02T18:47:30Z

src/components/Widget/components/Conversation/components/Messages/index.tsx

+import React, { useEffect } from 'react';
+import { useSelector, useDispatch } from 'react-redux';
+
+import { hideAvatar } from '../../../../../..//store/actions';


damfinkel · 2020-04-02T18:48:52Z

src/components/Widget/components/Conversation/components/Messages/index.tsx

+
+  let $message: HTMLDivElement | null;
+
+  // useEffect(() => scrollToBottom($message), [messages]);


should we remove this?

Not yet, I'll fix it in another PR

damfinkel · 2020-04-02T18:53:40Z

src/components/Widget/components/Conversation/components/Messages/index.tsx

+  // }
+
+  return (
+    <div id="messages" className="rcw-messages-container" ref={msg => $message = msg}>


I'd use the more "modern" way to create refs, which is the useRef hook:

const messageRef = useRef(null); ... return ( <div ... ref={messageRef}> ... )

Although what you did works, I personally prefer this.

damfinkel · 2020-04-02T18:58:23Z

src/components/Widget/components/Conversation/components/Sender/index.tsx

+}: Props) {
+  const input = React.createRef();
+
+  // componentDidUpdate() {


should we delete this?

I will fix it in another PR

damfinkel · 2020-04-02T18:58:49Z

src/components/Widget/components/Conversation/components/Sender/index.tsx

+    <form className="rcw-sender" onSubmit={sendMessage}>
+      <input type="text" className="rcw-new-message" name="message" placeholder={placeholder} disabled={disabledInput} autoFocus={autofocus} autoComplete="off"/>
+      <button type="submit" className="rcw-send">
+        <img src={send} className="rcw-send-icon" alt="send" />


this looks like it could be a ::before

damfinkel · 2020-04-02T18:59:18Z

src/components/Widget/components/Launcher/index.tsx

+
+import './style.scss';
+
+const openLauncher = require('../../../../../assets/launcher_button.svg') as string;


why require?

TypeScript needs it to be imported that way, or at least that's the simplest way I found to import SVGs

damfinkel · 2020-04-02T18:59:34Z

src/components/Widget/components/Launcher/index.tsx

+    <button type="button" className={chatOpened ? 'rcw-launcher rcw-hide-sm' : 'rcw-launcher'} onClick={toggle}>
+      <Badge badge={badge} />
+      {chatOpened ?
+        <img src={close} className="rcw-close-launcher" alt="" /> :


why no alt (next line too)?

Since it's an icon, I don't think it should have an alt

I think it should if it is part of a button without any text

damfinkel · 2020-04-02T18:59:53Z

src/components/Widget/index.tsx

@@ -54,7 +53,8 @@ function Widget({

  const onQuickButtonClicked = (event, value) => {
    event.preventDefault();
-    if (handleQuickButtonClicked) handleQuickButtonClicked(value);
+    handleQuickButtonClicked?.(value)
+    // if (handleQuickButtonClicked) handleQuickButtonClicked(value);


I didn't test correctly if the line above does really work. Let me try that first 😝

damfinkel · 2020-04-02T20:01:22Z

src/store/reducers/messagesReducer.ts

-  [actionTypes.HIDE_AVATAR]: (state, { index }) =>
-    state.update(index, message => message.set('showAvatar', false))
+  [HIDE_AVATAR]: (state: MessagesState, { index }) =>
+    state.messages[index].showAvatar = false


state should be immutable

I will add seamless-immutable in another PR

Still, I don't think we should leave it like this. I think it should work with just this:

messages = [...state.messages]; messages[index].showsAvatar = false; return { ...state, messages };

pabloferro · 2020-04-03T12:25:17Z

src/components/Widget/components/Conversation/components/Header/index.tsx

+    <div className="rcw-header">
+      {showCloseButton &&
+        <button className="rcw-close-button" onClick={toggleChat}>
+          <img src={close} className="rcw-close" alt="close" />


This is not related to this pull request, but the alt text should be translated. Do you have a way of passing locales or texts?

Maybe I can add a prop to manage that, but I think I'll need to add i18next and translate all alts to some extent 🤔 . Let me see if I can do it in another PR

pabloferro · 2020-04-03T12:41:42Z

src/components/Widget/components/Launcher/index.tsx

+const close = require('../../../../../assets/clear-button.svg') as string;
+
+type Props = {
+  toggle: () => {},


Shouldn't this be toggle: () => void,?

pabloferro · 2020-04-03T12:51:06Z

src/utils/createReducer.ts

 export const createReducer = <S>(
  reducer: { [key: string]: Function },
-  state: { [key: string]: S },
+  state: BehaviorState | MessagesState | QuickButtonsState,


Do you need this union?
Why don't you use S?

Because I didn't noticed 😝 . Fixed!

- change refs to use useRef hook - fix quick buttons feature, reducer and component - fix indentation issues - add timestamp to messages, now they will display underneath the message

- change the use on launcher of conditional classes - change state type in create reducer

mcallegari10 added 2 commits March 31, 2020 14:12

start typescript migration

a8b969b

- fix type issues in the redux store - bumped redux adn react-redux versions to latest

- migrate all the components to typescript

213c6cb

- change connect react-redux hoc for hooks - fix some store types issues

mcallegari10 requested review from pabloferro and damfinkel April 2, 2020 18:19

mcallegari10 assigned pabloferro and damfinkel Apr 2, 2020

damfinkel suggested changes Apr 2, 2020

View reviewed changes

damfinkel reviewed Apr 2, 2020

View reviewed changes

pabloferro reviewed Apr 3, 2020

View reviewed changes

pabloferro suggested changes Apr 3, 2020

View reviewed changes

mcallegari10 added 3 commits April 3, 2020 12:52

- remove unnecessary packages for prop types and immutable

1cf2853

- change refs to use useRef hook - fix quick buttons feature, reducer and component - fix indentation issues - add timestamp to messages, now they will display underneath the message

- change launcher on toggle function types

159f875

- change the use on launcher of conditional classes - change state type in create reducer

remove console log

241d0c9

pabloferro approved these changes Apr 3, 2020

View reviewed changes

damfinkel approved these changes Apr 3, 2020

View reviewed changes

mcallegari10 merged commit 04d2953 into react-update Apr 3, 2020

mcallegari10 deleted the ts-migration branch April 3, 2020 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeScript migration #163

TypeScript migration #163

mcallegari10 commented Apr 2, 2020

damfinkel Apr 2, 2020

mcallegari10 Apr 2, 2020

damfinkel Apr 2, 2020

pabloferro Apr 3, 2020

damfinkel Apr 2, 2020

damfinkel Apr 2, 2020

mcallegari10 Apr 2, 2020

damfinkel Apr 2, 2020

damfinkel Apr 2, 2020

mcallegari10 Apr 2, 2020

damfinkel Apr 2, 2020

damfinkel Apr 2, 2020

mcallegari10 Apr 3, 2020

damfinkel Apr 2, 2020

mcallegari10 Apr 2, 2020

pabloferro Apr 3, 2020

damfinkel Apr 2, 2020

mcallegari10 Apr 2, 2020

damfinkel Apr 2, 2020

mcallegari10 Apr 3, 2020

damfinkel Apr 3, 2020

pabloferro Apr 3, 2020

mcallegari10 Apr 3, 2020

pabloferro Apr 3, 2020

pabloferro Apr 3, 2020

mcallegari10 Apr 3, 2020

		@@ -0,0 +1,32 @@
		import React from 'react';

		const close = require('../../../../../../../assets/clear-button.svg') as string;


		let $message: HTMLDivElement \| null;

		// useEffect(() => scrollToBottom($message), [messages]);


		import './style.scss';

		const openLauncher = require('../../../../../assets/launcher_button.svg') as string;

TypeScript migration #163

TypeScript migration #163

Conversation

mcallegari10 commented Apr 2, 2020

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment