[stimulus-controller] fix bool attributes from being rendered incorrectly #121
Conversation
|
related tests pass - failures related to deprecation that is fixed in #122 |
| 'dataOrControllerName' => 'false-controller', | ||
| 'controllerValues' => ['isEnabled' => false], | ||
| 'expected' => 'data-controller="false-controller"', | ||
| ]; |
There was a problem hiding this comment.
I did some more digging into this... and I think my proposed solution (which you implemented perfectly) is not... perfect ;).
For this test case:
<!-- current output -->
<div
data-controller="false-controller"
data-false-controller-is-enabled-value=""
>The ="" for a false value is the problem. It should be ="false", which I've determined DOES cause the value in the controller to be false. Similarly, for true, the attribute should be ="true". Additionally, omitting the attribute entirely would cause this.hasIsEnabledValue (you can use a "hasser" like this to check for the existence of a value) to incorrectly return false with this current PR.
So I think the fix is to "fix" how we convert boolean values:
<!-- if isEnabled is passed as FALSE -->
<div
data-controller="false-controller"
data-false-controller-is-enabled-value="false"
>
<!-- if isEnabled is passed as TRUE -->
<div
data-controller="false-controller"
data-false-controller-is-enabled-value="true"
>
There was a problem hiding this comment.
that make sense.. done. keeping this "unresolved" for future reference.
jrushlow
force-pushed
the
fix/false-attribute
branch
2 times, most recently
from
242cabb to
1580cd8
Compare
| ], | ||
| 'controllerValues' => [], | ||
| 'expected' => 'data-controller="my-controller" data-my-controller-boolean-value="1" data-my-controller-number-value="4" data-my-controller-string-value="str"', | ||
| 'expected' => 'data-controller="my-controller" data-my-controller-boolean-value="true" data-my-controller-number-value="4" data-my-controller-string-value="str"', |
There was a problem hiding this comment.
i've implemented (string) true|false for the rendered value. But this "breaks" the previous 1 === true output even though false was blank.
I personally prefer true|false over 1|0 as it's clear that the value is not intended to be an int / (string) number. But would this be considered BC even though the PR is intended to fix false bool's?
There was a problem hiding this comment.
Do not think this is a BC break, because one who defined a Stimulus value as Boolean, and the value was true-thy, having it previously rendered as ..-value="1", which in Stimulus was correctly interpreted as true.
And fixing that the false value was not being rendered as ..-value="false" means imo it's currently incorrect, and of course the exact purpose of your PR ;].
|
Just hit this in my project, and wanted to let you know this PR fixed it for me :]. |
jrushlow
changed the title
Awesome! That's good to hear - we should be able to get this pushed out pretty quickly. |
weaverryan
force-pushed
the
fix/false-attribute
branch
from
1580cd8 to
2694dca
Compare
|
Thanks for the nice fix Jesse - and to @hvt also for double-checking it for us in a real app <3 |
3 participants
fixes #118