Refactor: Utilisation de TypeScript #51

maxime-gendron · 2019-08-12T14:31:27Z

J'ai intégré DE-55 dans ce PR.

christian-roy

Il reste quelques erreurs à corriger qui sortent avec la compilation TypeScript et TS Lint.

Pour sortir l'ensemble des erreurs de compilation tu peux installer TypeScript globalement avec yarn global add typescript et ensuite rouler tsc (ou tsc -w pour avoir le watch). Quand je le roule dans le package React ça me dit qu'il reste 141 erreurs.

Même chose pour TS Lint, que tu peux installer globalement avec yarn global add tslint et ensuite tu peux le rouler dans le package React avec tslint --project . pour sortir les erreurs. Il y en a une trentaine présentement.

meriouma · 2019-08-12T18:14:23Z

packages/react/src/components/forms/inputs/search-input.tsx

    const [{ value }, setValue] = useState({ value: '' });

-    const handleChange = event => {
+    const handleChange = (event: any) => {


event: ChangeEvent<HTMLInputElement> (import { ChangeEvent} from 'react')

Et c'était déjà de même, mais pas besoin de faire onChange={event => handleChange(event)} dans l'Input. Tu pourrais faire directement handleChange={handleChange} (même chose pour onKeyDown)

meriouma · 2019-08-12T18:18:07Z

packages/react/src/components/forms/inputs/search-input.tsx

@@ -98,7 +108,7 @@ const SearchInput = ({ disabled, hasButton, id, label, onChange, onSearch }) =>
        }
    };

-    const handleKeyDown = event => {
+    const handleKeyDown = (event: any) => {


event: KeyboardEvent<HTMLInputElement> (import { KeyboardEvent } from 'react')

meriouma · 2019-08-12T18:21:28Z

packages/react/src/components/forms/inputs/text-area.tsx

+    disabled?: boolean;
+    id: string;
+    label: string;
+    onBlur?: ((...args: any[]) => void);


onBlur?(value: string): void;

meriouma · 2019-08-12T18:21:44Z

packages/react/src/components/forms/inputs/text-area.tsx

+    id: string;
+    label: string;
+    onBlur?: ((...args: any[]) => void);
+    onChange?: ((...args: any[]) => void);


onChange?(value: string): void;

meriouma · 2019-08-12T18:21:54Z

packages/react/src/components/forms/inputs/text-area.tsx

+    label: string;
+    onBlur?: ((...args: any[]) => void);
+    onChange?: ((...args: any[]) => void);
+    onFocus?: ((...args: any[]) => void);


onFocus?(value: string): void;

meriouma · 2019-08-12T20:55:15Z

packages/react/src/components/media-view.tsx

+    minWidth?: number;
+}
+
+class MediaView extends Component<{}, {screenWidth: number, children?: Children}> {


Faire une interface pour le state et passer le type MediaViewProps comme type de props. Et children dans le state n'est pas utilisé.

interface State { screenWidth: number; } class MediaView extends Component<MediaViewProps, State> { [...] }

meriouma · 2019-08-12T20:56:39Z

packages/react/src/components/media-view.tsx

+}
+
+class MediaView extends Component<{}, {screenWidth: number, children?: Children}> {
+    constructor(props: any[]) {


props: MediaViewProps

meriouma · 2019-08-12T21:02:19Z

packages/react/src/components/media-view.tsx

@@ -37,3 +45,5 @@ export default class MediaView extends Component {
        return null;


Ligne 16 : Parenthèses inutiles, et espace de trop avant la virgule
Ligne 27 : Ça devrait être window.removeEventListener plutôt que window.addEventListener je pense.
componentDidMount, componentWillMount et handleScreeResize (typo) : devraient rien retourner. D'ailleurs le lint doit te dire de rajouter le type de retour : void et : ReactFragment | null pour la méthode render
Ligne 42 : children peut être undefined. Je suis pas certain que React aime ça

meriouma · 2019-08-12T21:04:58Z

packages/react/src/components/progress.tsx

+    value: number;
+}
+
+const getStep = ({ step, max, value }: GetStepProps) => {


L'interface n'est pas nécessaire je pense.

function getStep(step: number, max: number, value: number) : ReactElement {

meriouma · 2019-08-12T21:06:19Z

packages/react/src/components/progress.tsx

@@ -76,13 +83,13 @@ const getStep = (step, max, value) => {
    return <StepComponent key={step} />;
 };

-const Progress = ({ max, value }) => (
+const Progress = ({ max, value }: {max: number, value: number}) => (


interface pour les Props plutôt

meriouma · 2019-08-19T21:10:44Z

packages/react/package-lock.json

@@ -0,0 +1,6112 @@
+{
+  "name": "@equisoft/design-elements-react",


Supprime ce fichier, il faut utiliser yarn et non npm

meriouma · 2019-08-19T21:11:14Z

packages/react/package.json

@@ -32,6 +33,7 @@
    "url": "https://github.com/kronostechnologies/design-elements/issues"
  },
  "dependencies": {
+    "@types/lodash-es": "^4.17.3",


Devrait être dans les devDependencies

meriouma · 2019-08-19T21:19:29Z

packages/react/src/components/buttons/abstract-button.tsx

-import abstractStyle from './styles/abstract';
+import { styles } from './styles/abstract';
+
+export type Child = ReactNode | ReactNode[];


Supprime ce type, ReactNode inclut déjà ReactNode[]

meriouma · 2019-08-19T21:23:34Z

packages/react/src/components/forms/inputs/search-input.tsx

@@ -143,7 +153,7 @@ const SearchInput = ({ disabled, hasButton, id, label, onChange, onSearch }) =>
        (
            <SearchSubmit
                disabled={disabled}
-                type="submit"
+                className="primary"


C'est voulu le className?

Oui, il y a une propriété className sur le component searchButton.

meriouma · 2019-08-19T21:24:19Z

packages/react/src/components/forms/inputs/text-area.tsx

+            <StyledTextArea
+                disabled={disabled}
+                id={id}
+                onBlur={(event: FocusEvent<HTMLTextAreaElement>) => {handleBlur(event); }}


onBlur={handleBlur}

meriouma · 2019-08-19T21:24:30Z

packages/react/src/components/forms/inputs/text-area.tsx

+                disabled={disabled}
+                id={id}
+                onBlur={(event: FocusEvent<HTMLTextAreaElement>) => {handleBlur(event); }}
+                onChange={(event: ChangeEvent<HTMLTextAreaElement>) => {handleChange(event); }}


onChange={handleChange}

meriouma · 2019-08-19T21:30:45Z

packages/react/src/components/results/legend.tsx

+    color?: string;
+}
+
+const Legend = ({ items }: any) => (


Tu devrais créér une interface pour les props :

interface Props { items: Item[];

meriouma · 2019-08-19T21:31:09Z

packages/react/src/components/results/legend.tsx

    <List>
-        {items.map(item => (
+        {items.map((item: ItemProps) => (


En typant comme il faut les props et items, tu n'aurais pas besoin de spécifier item: ItemProps

meriouma · 2019-08-19T21:32:02Z

packages/react/src/components/results/progress-bar/progress-bar.tsx

  font-size: 0.875rem;
 `;

-const ProgressBar = ({ content }) => (
+interface ElProps {


Ça veut dire quoi El?

EL faisait référence à el(element) dans le map:

<React.Fragment> {content.map((el) => ( <div> <Label secondary={el.secondary || false}>{el.descriptionLabel}</Label> <Bar color={el.color} endLabel={el.endLabel} percent={el.percent} secondary={el.secondary || false} /> </div> ))} </React.Fragment>

J'ai opté pour une solution similaire à ce que tu as suggéré pour legend.tsx:

interface ProgressBarProps { content: {color: string, descriptionLabel: string, endLabel: string, percent: string, secondary?: boolean}[]; } const ProgressBar = ({ content }: ProgressBarProps) => (

Cela permet d'éliminer ElProps.

meriouma · 2019-08-19T21:32:23Z

packages/react/src/components/results/progress-bar/progress-bar.tsx

+    secondary: boolean;
+}
+
+const ProgressBar = ({ content }: any) => (


Fait une interface pour typer les props et content

meriouma · 2019-08-19T21:34:54Z

packages/react/tsconfig.json

-        "paths": {}
+        "paths": {},
+        "outDir": "./dist",
+        "declaration": true,


Faurait changer le plugin rollup pour https://github.com/ezolenko/rollup-plugin-typescript2, celui utilisé (https://github.com/rollup/rollup-plugin-typescript) supporte pas declaration: true. Quand on fait yarn build, ça ne fait que builder le bundle js dans dist/

Merci pour l'info! J'ai fait la conversion.

christian-roy

Gros refactor! 👍

Quelques modifications et questions. Les commentaires restants du review de Max M seraient aussi à corriger.

christian-roy · 2019-08-21T12:48:50Z

packages/react/package.json

@@ -58,10 +59,11 @@
    "eslint-plugin-import": "^2.16.0",
    "eslint-plugin-react": "^7.12.4",
    "husky": "^2.3.0",
+    "lodash-es": "^4.17.14",


Devrait être dans dependencies (à inverser avec @types/lodash-es).

christian-roy · 2019-08-21T12:51:51Z

.gitignore

@@ -4,3 +4,4 @@
 dist/
 node_modules/
 yarn-error.log
+.rpt2_cache/


Est-ce que c'est un dossier qui est créé automatiquement quand on génère les .d.ts?

C'est le folder de cache généré par Rollup.

christian-roy · 2019-08-21T12:52:43Z

packages/react/rollup.config.js

@@ -21,6 +21,8 @@ export default {
            exclude: 'node_modules/**',
        }),
        svgr({ icon: true }),
-        typescript(),
+        typescript({
+            objectHashIgnoreUnknownHack: true,


Est-ce que c'est parce qu'on avait cette erreur : ezolenko/rollup-plugin-typescript2#105 ?

Oui c'est exactement ça.

christian-roy · 2019-08-21T12:54:14Z

packages/react/src/components/buttons/abstract-button.tsx

-import abstractStyle from './styles/abstract';
+import { styles } from './styles/abstract';
+
+export type Child = ReactNode;


Si on fait juste un équivalent syntaxique, on ne serait pas mieux de laisser ReactNode et nous éviter de faire l'import de l'alias ailleurs dans l'application?

Je suis d'accord, je vais changer ça.

christian-roy · 2019-08-21T12:59:34Z

packages/react/src/components/buttons/abstract-button.tsx

@@ -1,16 +1,18 @@
 import { ReactNode } from 'react';
 import styled from 'styled-components';

-import abstractStyle from './styles/abstract';
+import { styles } from './styles/abstract';


Quand on importe le style de plusieurs fichiers différents ça crée des collision de noms de tous les nommes styles. J'ai commencé à leur donné des noms spécifiques, on pourrait probablement faire la même chose ici (quelque chose comme { abstractStyle }).

christian-roy · 2019-08-21T16:30:15Z

packages/react/src/components/media-view.tsx

+    screenWidth: number;
+}
+
+class MediaView extends Component<MediaViewProps, State> {


Devrait être refactored en Pure Function Component. Je crée une carte.

christian-roy · 2019-08-21T16:51:38Z

packages/react/src/components/results/progress-bar/bar/bar.tsx

@@ -6,7 +6,7 @@ const Container = styled.div `
  display: flex;
  margin-bottom: 1rem;
  p {
-    color: ${props => (props.secondary ? 'rgb(87, 102, 110)' : 'rgb(0, 0, 0)')};
+    color: ${(props: {secondary: boolean}) => (props.secondary ? 'rgb(87, 102, 110)' : 'rgb(0, 0, 0)')};


Est-ce qu'on est obligé de re-spécifier le type ici considérant que l'interface des props est déjà déclarée plus bas? On ne pourrais pas juste faire props: BarProps?

C'est un problème généré par l'utilisation de styled-components avec TypeScript. Si je met BarProps comme sur la component, la styled-component Container s'attend à recevoir toutes les components de BarProps ce qui génère une erreur TypeScript.

christian-roy · 2019-08-21T16:52:54Z

packages/react/src/components/results/progress-bar/progress-bar.tsx

+`;
+
+interface ProgressBarProps {
+    content: {color: string, descriptionLabel: string, endLabel: string, percent: string, secondary?: boolean}[];


Je pense que ça serait plus clean de créer une interface séparée pour le content et l'assigner ici.

Bonne idée! J'aurais pas pensé mettre une interface dans une interface.

christian-roy · 2019-08-21T16:59:23Z

packages/react/tsconfig.json

@@ -2,14 +2,17 @@
    "extends": "./node_modules/@equisoft/tslint-config/tsconfig.standards.json",
    "compilerOptions": {
        "target": "es6",
-        "module": "commonjs",
+        "module": "ESNext",


Pourquoi est-ce qu'on fait cette modif?

Nécessaire à Rollup quand il est utilisé avec https://github.com/ezolenko/rollup-plugin-typescript2.

christian-roy · 2019-08-21T17:02:38Z

packages/storybook/.storybook/config.js

@@ -1,5 +1,5 @@
 import { configure } from '@storybook/react';
-import '@equisoft/design-elements-web/style/body.css';


Laisser le link vers le fichier CSS. Je pense qu'il manquerait juste à update la version de @equisoft/design-elements-web dans le package.json de Storybook pour la dernière version (0.5.0) et le fichier CSS va être là.

J'ai update la version et ça fonctionne. J'ai remis le fichier .css dans config.js.

mgendron-equisofte added 5 commits August 9, 2019 11:34

buttons and inputs

53fb5c8

Forms

4a48d81

Headband

4fc5495

Other components

3ea90dd

Fixes

0bde61c

christian-roy suggested changes Aug 12, 2019

View reviewed changes

meriouma reviewed Aug 12, 2019

View reviewed changes

mgendron-equisofte added 9 commits August 13, 2019 10:41

TSC fixes

2671787

PR fixes

ecf4667

TS fixes

e2973e6

PR fixes

5f93e05

PR fixes

2f2fbad

PR fixes

c355ec0

Merge Master

f0dd2f4

New components refactor

8a34eb3

TS Typings Core setup

a8bcc35

meriouma reviewed Aug 19, 2019

View reviewed changes

meriouma requested changes Aug 19, 2019

View reviewed changes

mgendron-equisofte added 12 commits August 20, 2019 11:10

buttons and inputs

1fd91d9

Forms

0064990

Headband

b5740bb

Other components

ee0f877

Fixes

9579097

TSC fixes

408c732

PR fixes

cfd8c61

TS fixes

e95df60

PR fixes

71e20d5

PR fixes

5c5fc1f

PR fixes

c0ee19a

New components refactor

d23f445

TS Typings Core setup

5ce285d

christian-roy force-pushed the dev/DE-54 branch from a8bcc35 to 5ce285d Compare August 20, 2019 15:18

mgendron-equisofte and others added 5 commits August 20, 2019 14:06

PR fixes

2472272

Pull

6e613e6

wip

4b2ad39

renames Select.tsx to select.tsx

f11943d

PR fixes

2b72427

maxime-gendron force-pushed the dev/DE-54 branch from 94048c8 to 2b72427 Compare August 20, 2019 18:56

mgendron-equisofte added 5 commits August 21, 2019 08:22

Rollup config/plugin update

8ca26c4

TypeScript in chooser.tsx

e4905b1

Rollup changes fixes

35215b2

Changed Child exports/imports for ReactNode

dffa255

Fixes chooser.tsx

c6e8745

christian-roy suggested changes Aug 21, 2019

View reviewed changes

mgendron-equisofte and others added 5 commits August 21, 2019 13:41

package.json update

6be2984

styles imports syntax update

c85e33b

progress-bar.tsx update

f000ba3

config.js update

69e2730

Review Fixes

2f36bd5

christian-roy merged commit 5371cb7 into master Aug 21, 2019

christian-roy deleted the dev/DE-54 branch August 22, 2019 18:27

savutsang mentioned this pull request Nov 27, 2023

feat(Table): add Footer #640

Merged

		@@ -37,3 +45,5 @@ export default class MediaView extends Component {
		return null;

		@@ -0,0 +1,6112 @@
		{
		"name": "@equisoft/design-elements-react",

		@@ -1,5 +1,5 @@
		import { configure } from '@storybook/react';
		import '@equisoft/design-elements-web/style/body.css';

Refactor: Utilisation de TypeScript #51

Refactor: Utilisation de TypeScript #51

Conversation

maxime-gendron commented Aug 12, 2019 • edited Loading

christian-roy left a 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

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

christian-roy left a 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

maxime-gendron commented Aug 12, 2019 •

edited

Loading