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

[BUGFIX beta] fix regression of clicking link-to with disabled=true #15952

Merged
merged 3 commits into from
Dec 22, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 19 additions & 31 deletions packages/ember-glimmer/lib/components/link-to.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,25 +72,9 @@
{{/link-to}}
```
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`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"except undefined" can be removed I think, as undefined is not truthy anyways?

Copy link
Member

Choose a reason for hiding this comment

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

GAH! Thank you

```javascript
import LinkComponent from '@ember/routing/link-component';
import { computed } from '@ember/object';
LinkComponent.reopen({
disabled: computed(function(key, value) {
if (value !== undefined) {
this.set('_isDisabled', value === true);
}
return value === true ? get(this, 'disabledClass') : false;
})
});
```
see "Overriding Application-wide Defaults" for more.
See "Overriding Application-wide Defaults" for more.
### Handling `href`
`{{link-to}}` will use your application's Router to
Expand Down Expand Up @@ -277,30 +261,31 @@
check out inherited properties of `LinkComponent`.
### Overriding Application-wide Defaults
``{{link-to}}`` creates an instance of `LinkComponent`
for rendering. To override options for your entire
application, reopen `LinkComponent` and supply the
desired values:
``` javascript
``{{link-to}}`` creates an instance of `LinkComponent` for rendering. To
override options for your entire application, export your customized
`LinkComponent` from `app/components/link-to.js` with the desired overrides:
```javascript
// app/components/link-to.js
import LinkComponent from '@ember/routing/link-component';
LinkComponent.reopen({
export default LinkComponent.extend({
activeClass: "is-active",
tagName: 'li'
})
```
It is also possible to override the default event in
this manner:
It is also possible to override the default event in this manner:
``` javascript
```javascript
import LinkCompoennt from '@ember/routing/link-component';
LinkComponent.reopen({
export default LinkComponent.extend({
eventName: 'customEventName'
});
```
@method link-to
@for Ember.Templates.helpers
@param {String} routeName
Expand Down Expand Up @@ -515,7 +500,6 @@ const LinkComponent = EmberComponent.extend({
*/
init() {
this._super(...arguments);
this._isDisabled = false;

// Map desired event name to invoke function
let eventName = get(this, 'eventName');
Expand All @@ -534,10 +518,14 @@ const LinkComponent = EmberComponent.extend({
*/
disabled: computed({
get(_key: string): boolean {
// always returns false for `get` because (due to the `set` just below)
// the cached return value from the set will prevent this getter from _ever_
// being called after a set has occured
return false;
},

set(_key: string, value: any): boolean {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated to the changes here, but the typing seems wrong. Shouldn't it be set(...): string|false?

Copy link
Member

Choose a reason for hiding this comment

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

@simonihmig - Yes, this should be typed as a string | null I believe.

if (value !== undefined) { this.set('_isDisabled', value); }
this._isDisabled = value;
Copy link
Contributor Author

@simonihmig simonihmig Dec 22, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

@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...


return value ? get(this, 'disabledClass') : false;
},
Expand Down Expand Up @@ -636,7 +624,7 @@ const LinkComponent = EmberComponent.extend({

if (get(this, 'bubbles') === false) { event.stopPropagation(); }

if (get(this, '_isDisabled')) { return false; }
if (this._isDisabled) { return false; }

if (get(this, 'loading')) {
// tslint:disable-next-line:max-line-length
Expand Down
13 changes: 12 additions & 1 deletion packages/ember/tests/helpers/link_to_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ moduleFor('The {{link-to}} helper - basic tests', class extends ApplicationTestC
assert.equal(this.$('#about-link.do-not-want').length, 1, 'The link can apply a custom disabled class via bound param');
}

[`@test the {{link-to}} helper does not respond to clicks when disabled`](assert) {
[`@test the {{link-to}} helper does not respond to clicks when disabledWhen`](assert) {
this.addTemplate('index', `
{{#link-to "about" id="about-link" disabledWhen=true}}About{{/link-to}}
`);
Expand All @@ -144,6 +144,17 @@ moduleFor('The {{link-to}} helper - basic tests', class extends ApplicationTestC
assert.equal(this.$('h3:contains(About)').length, 0, 'Transitioning did not occur');
}

[`@test the {{link-to}} helper does not respond to clicks when disabled`](assert) {
this.addTemplate('index', `
{{#link-to "about" id="about-link" disabled=true}}About{{/link-to}}
`);

this.visit('/');
this.click('#about-link');

assert.equal(this.$('h3:contains(About)').length, 0, 'Transitioning did not occur');
}

[`@test the {{link-to}} helper responds to clicks according to its disabledWhen bound param`](assert) {
this.addTemplate('index', `
{{#link-to "about" id="about-link" disabledWhen=disabledWhen}}About{{/link-to}}
Expand Down