[BUGFIX beta] fix regression of clicking link-to with disabled=true #15952
Conversation
|
I think |
|
Hm, not sure...
So I would say either remove it all together (no-go IMHO), or fix the bug!? |
|
Agreed. We should fix this. Even if we decide later to deprecate. |
|
Anything missing here? /cc @rwjblue |
The prior system was overly complicated (it still kinda is, but thats a story for a future refactor). It relied on the fact that `_isDisabled` was set to disable the link-to (either via hitting the `disabled` CPs setter, or via passing `disabledWhen`) and then checked `_isDisabled` when being invoked. The original issue was that we were _always_ setting `_isDisabled` to false in the constructor (done to ensure `link-to` has a consistent shape). That was fixed by pre-sloting `_isDisabled` _before_ calling `this._super` (yes, I know thats generally bad, but so is our `Object.assign(instance, properties)` that runs _before_ `init`).
rwjblue
force-pushed
the
fix-linkto-disabled
branch
from
52527e0 to
c5b3626
Compare
|
@simonihmig - Just updated the implementation here, mind taking another look? |
| return false; | ||
| }, | ||
|
|
||
| set(_key: string, value: any): boolean { |
There was a problem hiding this comment.
Unrelated to the changes here, but the typing seems wrong. Shouldn't it be set(...): string|false?
There was a problem hiding this comment.
@simonihmig - Yes, this should be typed as a string | null I believe.
| any passed value to `disabled` will disable it except `undefined`. | ||
| to ensure that only `true` disable the `link-to` component you can | ||
| override the global behavior of `LinkComponent`. | ||
| any truthy value passed to `disabled` will disable it except `undefined`. |
There was a problem hiding this comment.
"except undefined" can be removed I think, as undefined is not truthy anyways?
| set(_key: string, value: any): boolean { | ||
| if (value !== undefined) { this.set('_isDisabled', value); } | ||
| this._isDisabled = value; |
There was a problem hiding this comment.
The changes here seem right to me in general, but they do change the semantics a bit, don't they (the truthy vs. not undefined)? Could they break something for somebody?
There was a problem hiding this comment.
@simonihmig if you take a look at where this._isDisabled is actually checked (in _invoke), we have been doing a truthy check there for a while so changing this to always update it just makes it simpler...
|
@rwjblue Uh, already merged? :) I added a few comments... |
#15759 introduced a regression, allowing clicking on a
{{#link-to "somewhere" disabled=true}}to still invoke the transition. This was because the default value of_isDisabledwas moved to the init hook here: 8c68f5b#diff-49415576dd428811b5854523cc9b9adcR528, effectively overriding any previously set value.The added test covers this, and was failing before the change here.