Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(MoneyInput): Ajoute un money-input qui formate la valeur entrée en fonction de la langue #118

Merged
merged 19 commits into from
Apr 23, 2020

Conversation

maxime-gendron
Copy link
Contributor

@maxime-gendron maxime-gendron commented Mar 10, 2020

PR Description

Ce component a été initialement créé dans le repo de Dabench (lien). J'ai adapté le component aux besoins du Design System, donc il y à certaines différences avec le currency-input de Dabench.

La principale fonctionnalité de ce component est de formater les valeurs entrées selon la langue définie dans les props. Par exemple:

<MoneyInput language="en" value={10}/> // $10
<MoneyInput language="fr" value={10}/> // 10 $

Pour la largeur de l'input, j'ai regardé avec @maboilard et on change la valeur de 116px des maquettes pour 128px. Les styles mobiles ne sont pas fait, car ce component est basé sur text-input. Un PR sera fait pour mettre les styles mobiles sur tous les inputs.

New component checklist

  • The new component and its tests are in the same component folder.
  • The component is unit tested and/or snapshot tested.
  • All of the relevant Storybook stories have been added to the storybook package.
  • There are no linting errors or warnings in the modified/new code.
  • The CircleCI build is successful.

@maxime-gendron maxime-gendron changed the title feat(MoneyInput): Ajoute un money-input qui formate la valeur entrée tout dépendant de la langue feat(MoneyInput): Ajoute un money-input qui formate la valeur entrée en fonction de la langue Mar 10, 2020
Copy link
Contributor

@LarryMatte LarryMatte left a comment

Choose a reason for hiding this comment

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

Un peu difficile à tester ici, mais en mobile, il faudrait que lorsque l'utilisateur va triggers l'input, que ce soit le clavier avec seulement les chiffres qui apparaissent.

packages/react/src/components/label/label.tsx Show resolved Hide resolved
const InputWrapper = styled.div<{language: Language}>`
input {
text-align: ${({ language }) => language === 'en' ? 'left' : 'right'};
width: 128px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Est-ce qu'on veut vraiment avoir une largeur fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai regardé avec @maboilard et idéalement on aurait un min-width et si la valeur entrée excède cette largeur l'input s'adapte. Je vais regarder ce que je peux faire pour ça. J'avais essayé l'autre fois et c'était weird avec text-input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai réussi à figure out une solution pour que la largeur de l'input devienne dynamique en haut d'un certain nombre de caractères. J'ai donc ramené la largeur par défaut comme sur les maquettes (116px).

Copy link
Contributor

@LarryMatte LarryMatte Mar 16, 2020

Choose a reason for hiding this comment

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

hum pas certain que ce soit le comportement qu'on veut avoir.
Il se passerait quoi si l'input devient plus grand que son container?
Je ne sais pas si c'est la bonne solution, mais est-ce que la largeur pourrait être une props ? ou est-ce qu'on pourrait définir le nombre de chiffres qui est sensé s'afficher dans l'input et la largeur serait défini par ça?
Avoir un champ qui s'élargie selon son contenu n'est pas top selon moi.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je verrais 3 solutions possibles:

  • Soit j'expose un prop width en laissant la valeur par défaut à 116px. Le principal downside de cette solution est qu'il y a des bonnes chances que le user qui set sa largeur lui même ne respect pas notre 8px grid.
  • Soit je laisse l'input à largeur dynamique, mais je limite sa largeur maximale à 100% de sont container.
  • Soit je met une largeur fixe.

Qu'est-ce qui serait mieux selon toi?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On pourrait faire des presets de largeur. Par exemple:

  • "small": 116px;
  • "medium": 132px;
  • "large": 148px;
  • "100%": 100%;

On met small par defaut. De cette manière on garde quand même un certain contrôle sur les largeurs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ça aurait du sens pour moi, faut voir ce que @jni- en pense.

Copy link
Contributor

Choose a reason for hiding this comment

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

si y'a un 100% pour laisser le controle au container, pourquoi pas. Tant qu'à y aller avec les noms, j'irais p-t pour small, medium, large, stretch ou de quoi du genre.

Par contre ideallement ca fitterait avec le reste. Si j'ai un formulaire vertical avec 5 champs, est-ce que les autre input (pas money) vont être de la même largeur? Si non, ca manque un peu d'unité dans le design system je trouve. Mais si je peux mettre 100% et décider la largeur de tous les inputs avec le container, alors c,est un moindre mal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai regardé ça avec @maboilard et on met ça fix à 132px.

Copy link
Contributor

Choose a reason for hiding this comment

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

Si jamais on veut changer pour ce input dans Dabench plutôt que notre copie, faudrait que vous repassiez sur les maquettes de Dabench. On avait 232px en desktop/tablette, et le champ s'ajustait à la taille de l'écran en mobile (~100%)

packages/react/src/components/money-input/money-input.tsx Outdated Show resolved Hide resolved
nextDisplayValue = roundedValue.toString();
} else {
const mask: RegExp = precision > 0 ? /[^0-9.,]/g : /[^0-9]/g;
nextDisplayValue = rawValue.replace(mask, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

ca serait pas mieux de le repartir comme un number? parce que la le rounding arrivera pasdans ce cas-ci

Copy link
Contributor Author

Choose a reason for hiding this comment

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

La value est formattée onBlur avec updateFormattedValue(), donc au final la valeur devient un nombre. Je laisserais ça comme ça pour laisser le user entrer un peu ce qu'il veut et on handle la valeur par la suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ca peut etre OK de dire "si tu mets d'la scrap tanpis", mais juste faire attention à ne pas avoir au fil du temps tout le temps plein d'edge case. Ce genre de truc fait que partout ailleurs tout le monde doit géré la scrap qui n'a pas été gérée en amont, personne va venir revoir ici pour le réglé à la source (trust me lol). C'est pas that bad, juste un petit smell basé sur l'expérience lol

Aussi, ca peut clashé pour l'utilisateur d'avoir 2 facon de le géré selon si le component "give up" ou non :P

packages/react/src/components/money-input/money-input.tsx Outdated Show resolved Hide resolved
packages/react/src/components/money-input/money-input.tsx Outdated Show resolved Hide resolved
packages/react/src/utils/currency.ts Outdated Show resolved Hide resolved
packages/react/src/utils/currency.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@LarryMatte LarryMatte left a comment

Choose a reason for hiding this comment

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

LGTM, pour les sections qui me concerne du moins.

setHasFocus(true);
}

const getRoundedValue = (val: string): number | null => {
Copy link
Contributor

Choose a reason for hiding this comment

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

function à place de const pour être pareil. peut-etre aussi parseAndRound ou formatValue ou de quoi du genre pour dire que la différence avec roundValue c'est que ca prend une str

@maxime-gendron maxime-gendron merged commit f50a2af into master Apr 23, 2020
@maxime-gendron maxime-gendron deleted the dev/DE-67 branch April 24, 2020 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants