Don't set read-only class if readonly is null#276
Open
cincodenada wants to merge 2 commits intoSemantic-Org:masterfrom
Open
Don't set read-only class if readonly is null#276cincodenada wants to merge 2 commits intoSemantic-Org:masterfrom
cincodenada wants to merge 2 commits intoSemantic-Org:masterfrom
Conversation
toggleClass() requires a boolean value, not just a truthy/falsy one. When readonly is null, the current code results in the read-only class being toggled (since null is treated the same as the argument not being provided), resutling in indeterminate results depending on what the previous value of readonly was. To avoid this surprising behavior, just coerce the value of readonly to a boolean before passing it to toggleClass(), so that null will always result in no read-only class, same as if the readonly attribute was not provided.
cincodenada
force-pushed
the
fix-null-readonly
branch
from
80e04d4 to
18244ed
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
toggleClass() requires a boolean value, not just a truthy/falsy one.
Because the current code just passes in the value of readonly directly, if it is null, the current code results in the read-only class toggled rather than set to any determinate value. While setting readonly to null may be an edge case, it's easy to make this behavior less surprising and unintuitive by simply coercing the value passed to toggleClass to a boolean, so that readonly=null always results in no read-only class.
I've created an Ember Twiddle demonstrating the current behavior here (it may take some time to build, I'm not sure where that build is cached): https://ember-twiddle.com/b9967c2e3604add699c58a2f97399915?openFiles=templates.application%5C.hbs%2C
You can see in the demonstration that true and false act as expected, but
nullbehaves strangely - if you selecttrueand thennull, the checkbox will be toggleable, but if you selectfalseand thennull, it will be read-only. This is because setting readonly tonullhas the effect of toggling it, instead of setting it to any given value.