[FEATURE] Mieux gérer les valeurs acceptées d'isDisabled des Checkbox/Radio
#638
Conversation
|
Une fois l'application déployée, elle sera accessible à cette adresse https://ui-pr638.review.pix.fr |
yannbertrand
force-pushed
the
improve-checkbox-radio-disabled-docs
branch
from
ef5f768
to
6f93ac2
Compare
yannbertrand
changed the title
isDisabled des CheckboxisDisabled des Checkbox/Radio
yannbertrand
force-pushed
the
improve-checkbox-radio-disabled-docs
branch
from
c6b2fb0
to
bb436ac
Compare
addon/components/pix-checkbox.js
Outdated
| if (['false', 'null', 'undefined'].includes(this.args.isDisabled)) { | ||
| console.warn( | ||
| 'PixCheckbox: @isDisabled attribute should be a boolean, a string was passed. Checkbox is considered disabled.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
thought :
Je suis pas fan d'ajouer ce genre de chose. car ça part en prod et c'est du code "inutil" pour l'usage final .
Ce genre de chose doit être valider lors de nos tests d'implem du composant. vérifier que le composant est bien disabled ou non dans nos cas d'usage. sinon on peut faire ça pour tout les @ qui prennent un boolean en paramètre ? 🤔
There was a problem hiding this comment.
Merci pour le partage 👍
On utilise déjà ce genre de chose dans PixMessage, ou des throw dans plusieurs components
En fait on aimerai bien s'éviter des futures erreurs (on a passé 1h dessus hier 😬). On peut être plus strict en throwant si besoin, mais même résultat pour l'usage interne ^^"
IMO ça reste un if sans trop d'impact et ça peut nous aider pas mal !
There was a problem hiding this comment.
Je ne suis pas fan non plus des throw (j'ai tendance à les supprimer quand je les vois) qui partent aussi en prod. qui sont au final des cas qui n'arriveront jamais.
J'associe ça a du code pour nous aider a debugger (parcequ'on a mal lu la signature de PixUI, ou qu'on a mal écrite la doc du composant). Du coup à mon sens c'est du code "mort" puisqu'en production, on ne rentrera jamais dedans.
There was a problem hiding this comment.
Je crois avoir trouvé quelque chose pour répondre à nos deux problématiques !
Dans @ember/debug (officiel donc), il y a des méthodes de logs qui sont supprimées lors du build de production : https://api.emberjs.com/ember/5.8/classes/@ember%2Fdebug/methods/warn?anchor=warn.
Ça me semble être le meilleur de 2 mondes, non ? 😄
There was a problem hiding this comment.
là je dis OUI (à tester )
There was a problem hiding this comment.
Ça fonctionne ! 1024pix/pix#8940 ✨ 🎉
|
Func ok aussi |
yannbertrand
force-pushed
the
improve-checkbox-radio-disabled-docs
branch
from
bb436ac
to
8a67c4e
Compare
addon/components/pix-checkbox.js
Outdated
| return this.args.isDisabled || this.args.disabled; | ||
| warn( | ||
| 'PixCheckbox: @isDisabled attribute should be a boolean, a string was passed. Checkbox is considered disabled.', | ||
| ['false', 'null', 'undefined'].includes(this.args.isDisabled), |
There was a problem hiding this comment.
thought : ![false,true].includes(this.args.isDisabled) serait plus clair. parce que l'on pourrait passer toto et le warn ne serait pas executé
There was a problem hiding this comment.
Yes, j'ai aussi ajouté undefined car ça générait des warnings dans les cas de figure :
<PixCheckbox>(=>undefined)<PixCheckbox @isDisabled={{undefined}}>
Pour être complet j'ai aussi ajouté null pour être plus permissif
<PixCheckbox @isDisabled={{null}}>
WDYT ?
yannbertrand
force-pushed
the
improve-checkbox-radio-disabled-docs
branch
6 times, most recently
from
17e5c76
to
3906f98
Compare
yannbertrand
force-pushed
the
improve-checkbox-radio-disabled-docs
branch
from
3906f98
to
2e02e50
Compare
yannbertrand
force-pushed
the
improve-checkbox-radio-disabled-docs
branch
from
2e02e50
to
def54df
Compare
pix-service-auto-merge
force-pushed
the
improve-checkbox-radio-disabled-docs
branch
from
def54df
to
58c39f1
Compare
# [46.1.0](v46.0.3...v46.1.0) (2024-05-17) ### 🚀 Amélioration - [#638](#638) Mieux gérer les valeurs acceptées d'`isDisabled` des Checkbox/Radio
|
🎉 This PR is included in version 46.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎄 Problème
Pour le moment on peut se retrouver avec
aria-disabled=""selon ce qu'on passe au composant. L'attributaria-disabledavec une valeur autre que"true"ne considère pas l'input comme "disabled" pour VoiceOver qui n'énonce pas "dimmed checkbox". C'est trompeur pour l'usager.🎁 Proposition
S'assurer de bien transmettre la valeur
"true". On s'est aligné sur les contrats d'interface attendus pardisabledpar exemple, càd n'importe quelle string considère que c'est "true". Voir https://stackblitz.com/edit/ember-cli-editor-output-pauj7r?file=app%2Ftemplates%2Fapplication.hbsUn warning en environnement de dev a été ajouté en cas d'erreur potentielle. Ce warning ne s'exécutera pas dans un environnement de production (
NODE_ENV=production) grâce à@ember/debug.On a ajouté beaucoup de cas de tests pour s'assurer de ne pas casser les multiples cas d'usages dans le futur.
🌟 Remarques
À noter un comportement tricky de testing-library : une checkbox
aria-disabledest "clickable" sans que ça ne throw, mais rien ne se passe (comme prévu).🎅 Pour tester
S'assurer que les différents états de la Checkbox fonctionnent correctement, y compris avec VoiceOver.
S'assurer que les différents états du RadioButton fonctionnent correctement, y compris avec VoiceOver.