Skip to content
This repository has been archived by the owner on Mar 10, 2022. It is now read-only.

CheckboxInput: Typescript migration #1520

Merged
merged 2 commits into from
Oct 14, 2021

Conversation

rlecellier
Copy link
Contributor

Afin de spliter OfferForm en morceaux plus lisible en utilisant Typescript, j'ai besoin que ce petit composant soit migré.

@rlecellier rlecellier force-pushed the rlecellier/CheckboxInput_typescript branch 2 times, most recently from 81e08ad to a295cee Compare October 12, 2021 07:51
@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #1520 (085c4fe) into master (5674e04) will increase coverage by 0.01%.
The diff coverage is 92.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1520      +/-   ##
==========================================
+ Coverage   87.89%   87.90%   +0.01%     
==========================================
  Files         378      378              
  Lines        6410     6417       +7     
  Branches     1751     1759       +8     
==========================================
+ Hits         5634     5641       +7     
  Misses        748      748              
  Partials       28       28              
Impacted Files Coverage Δ
...ithdrawalDetailsFields/WithdrawalDetailsFields.jsx 100.00% <ø> (ø)
...sibilityCheckboxList/AccessibilityCheckboxList.jsx 96.77% <ø> (ø)
.../Offers/Offer/OfferDetails/OfferForm/OfferForm.jsx 98.07% <ø> (ø)
...eguide/StyleguideElements/StyleguideCheckboxes.jsx 20.00% <ø> (ø)
...ents/layout/inputs/CheckboxInput/CheckboxInput.tsx 92.59% <92.59%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5674e04...085c4fe. Read the comment docs.

Copy link
Contributor

@gael-boyenval gael-boyenval left a comment

Choose a reason for hiding this comment

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

à voir en fonction de eslint concernant les null | undefined et les default objects properties

import React from 'react'

interface IProps {
SvgElement?: React.ComponentType | null,
Copy link
Contributor

Choose a reason for hiding this comment

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

tu n'as pas besoin du null car la prop optionnelle => ça équivaut à React.ComponentType | undefined

Suggested change
SvgElement?: React.ComponentType | null,
SvgElement?: React.ComponentType,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok on à deux manière de faire :

// le type 
propName?: propType

// props par defaut
propName = undefined

ou bien

// le type 
propName?: propType | null

// props par defaut
propName = null

J'aime bien le null explicite.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, ça me va, mais en gros on type un truc qui peut être null / undefined / autreType mais qui ne sera jamais undefined

Copy link
Contributor Author

@rlecellier rlecellier Oct 12, 2021

Choose a reason for hiding this comment

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

Ha oui bien vu.

Du coup quelque-chose comme ca conviendrait mieux ?

interface IProps {
  SvgElement?: React.ComponentType | null,
  checked: boolean,
  className: string,
  disabled: boolean,
  hiddenLabel: boolean,
  isInError: boolean,
  isLabelDisable: boolean,
  label: string,
  name: string,
  onChange: () => void,
  subLabel: string | null,
}

export const CheckboxInput = ({
  SvgElement = null,
  checked = false,
  className = '',
  disabled = false,
  hiddenLabel = false,
  isInError = false,
  isLabelDisable = false,
  label,
  name,
  onChange,
  subLabel = null,
}: IProps) => {

isLabelDisable?: boolean,
label: string,
name: string,
onChange: () => void,
Copy link
Contributor

Choose a reason for hiding this comment

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

je pense que onChange prend un param

Suggested change
onChange: () => void,
onChange: (event: React.ChangeEvent<HTMLInputElement>) => void,

@rlecellier rlecellier force-pushed the rlecellier/CheckboxInput_typescript branch from a295cee to 085c4fe Compare October 13, 2021 12:37
@rlecellier rlecellier merged commit fc2425c into master Oct 14, 2021
@rlecellier rlecellier deleted the rlecellier/CheckboxInput_typescript branch October 14, 2021 13:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants