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

Refactor Nav/Navbar/Dropdown's LinkTo components to not extend from Ember's LinkComponent #1455

Merged
merged 10 commits into from May 14, 2021
8 changes: 4 additions & 4 deletions addon/components/bs-dropdown/menu.hbs
Expand Up @@ -17,8 +17,8 @@
{{yield
(hash
item=(bs-default @itemComponent (component "bs-dropdown/menu/item"))
link-to=(bs-default @linkToComponent (component "bs-dropdown/menu/link-to"))
linkTo=(bs-default @linkToComponent (component "bs-dropdown/menu/link-to"))
link-to=(bs-default @linkToComponent (component "bs-link-to" class=(if (macroCondition (macroGetOwnConfig "isNotBS3")) "dropdown-item")))
linkTo=(bs-default @linkToComponent (component "bs-link-to" class=(if (macroCondition (macroGetOwnConfig "isNotBS3")) "dropdown-item")))
divider=(bs-default @dividerComponent (component "bs-dropdown/menu/divider"))
)
}}
Expand Down Expand Up @@ -46,8 +46,8 @@
{{yield
(hash
item=(bs-default @itemComponent (component "bs-dropdown/menu/item"))
link-to=(bs-default @linkToComponent (component "bs-dropdown/menu/link-to"))
linkTo=(bs-default @linkToComponent (component "bs-dropdown/menu/link-to"))
link-to=(bs-default @linkToComponent (component "bs-link-to" class=(if (macroCondition (macroGetOwnConfig "isNotBS3")) "dropdown-item")))
linkTo=(bs-default @linkToComponent (component "bs-link-to" class=(if (macroCondition (macroGetOwnConfig "isNotBS3")) "dropdown-item")))
divider=(bs-default @dividerComponent (component "bs-dropdown/menu/divider"))
)
}}
Expand Down
15 changes: 0 additions & 15 deletions addon/components/bs-dropdown/menu/link-to.js

This file was deleted.

10 changes: 10 additions & 0 deletions addon/components/bs-link-to.hbs
@@ -0,0 +1,10 @@
<LinkTo
@route={{this.route}}
@models={{this._models}}
@query={{this._query}}
@disabled={{@disabled}}
class={{@class}}
...attributes
>
{{yield}}
</LinkTo>
110 changes: 110 additions & 0 deletions addon/components/bs-link-to.js
@@ -0,0 +1,110 @@
/* eslint-disable ember/classic-decorator-no-classic-methods */
import Component from '@ember/component';
import { tagName } from '@ember-decorators/component';
import { inject as service } from '@ember/service';
import { assert } from '@ember/debug';
import ComponentChild from '../mixins/component-child';
import { dependentKeyCompat } from '@ember/object/compat';

/**
This is largely copied from Ember.LinkComponent. It is used as extending from Ember.LinkComponent has been deprecated.
We need this to
* register ourselves to a parent component that needs to know `active` state due to Bootstrap markup requirements, see Nav/LinkTo
* continue supporting positional params until we can remove support

@class LinkComponent
@namespace Components
@extends Component
@private
*/
@tagName('')
class LinkComponent extends Component.extend(ComponentChild) {
@service('router')
router;

@dependentKeyCompat
get active() {
if (!this.route) {
return false;
}

// Not all Ember versions we support correctly entangle autotracking with routing state changes, so we manually do that here
simonihmig marked this conversation as resolved.
Show resolved Hide resolved
// See https://github.com/emberjs/ember.js/issues/19004
// shamelessly stolen from https://github.com/rwjblue/ember-router-helpers/blob/master/addon/utils/track-active-route.js

// ensure we recompute anytime `router.currentURL` changes
this.router.currentURL;

// ensure we recompute whenever the `router.currentRouteName` changes
// this is slightly overlapping with router.currentURL but there are
// cases where route.currentURL doesn't change but the
// router.currentRouteName has (e.g. loading and error states)
this.router.currentRouteName;

return this.router.isActive(this.route, ...this._models, { queryParams: this._query });
}

get _models() {
let { model, models } = this;

if (model !== undefined) {
return [model];
} else if (models !== undefined) {
assert('The `@models` argument must be an array.', Array.isArray(models));
return models;
} else {
return [];
}
}

get _query() {
return this.query ?? {};
}

// eslint-disable-next-line ember/no-component-lifecycle-hooks
didReceiveAttrs() {
super.didReceiveAttrs(...arguments);

let { params } = this;
if (!params || params.length === 0) {
return;
}

params = params.slice();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We haven't formally deprecated positional params for link params it seems. While this is now deprecated for Ember's original LinkComponent, we do not trigger this deprecation anymore. So we should deprecate this explicitly by ourselves I think. Should be a follow up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't we dropped support for curly-bracket component invocation in v4? If so, there shouldn't be a public API to provide positional params to a component. Or did I missed something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't we dropped support for curly-bracket component invocation in v4

We haven't declared so, no. We dropped arguments for HTML attributes, and more or less recommended to use angle brackets, but haven't explicitly stopped support for curly invocation. At the time there was no reason for this, and this is still not deprecated from Ember's side AFAIK. Except for this positional arguments issue there is no reason why curly invocation should bother us, as long as Ember supports it we can basically not care about it.

We can choose to drop support in a next major, or just say that the use of our custom link component drops support for positional arguments and all the other things (currentWhen) that accidentally became part of our implementation due to extending from LinkComponent, whether or not we consider this a public API or not. (it's a "gray area" I guess)

IIRC you were more on the side of accepting major versions more often, right? So should we actually do this refactoring as part of a v5, with just this edge case-y thing as the (potentially, but rather rare) breaking change?


// taken from original Ember.LnkComponent
// Process the positional arguments, in order.

// Skipping this, as we don't support this
// 1. Inline link title comes first, if present.
// if (!hasBlock) {
// this.set('linkTitle', params.shift());
// }

// 2. The last argument is possibly the `query` object.
let queryParams = params[params.length - 1];

if (queryParams && queryParams.isQueryParams) {
this.set('query', params.pop().values);
} else {
this.set('query', undefined);
}

// 3. If there is a `route`, it is now at index 0.
if (params.length === 0) {
this.set('route', undefined);
} else {
this.set('route', params.shift());
}

// 4. Any remaining indices (if any) are `models`.
this.set('model', undefined);
this.set('models', params);
}
}

LinkComponent.reopenClass({
positionalParams: 'params',
});

export default LinkComponent;
5 changes: 2 additions & 3 deletions addon/components/bs-nav.hbs
Expand Up @@ -2,10 +2,9 @@
{{yield
(hash
item=(bs-default @itemComponent (component "bs-nav/item"))
link-to=(bs-default @linkToComponent (component "bs-nav/link-to"))
linkTo=(bs-default @linkToComponent (component "bs-nav/link-to"))
link-to=(bs-default @linkToComponent (component "bs-link-to" class=(if (macroCondition (macroGetOwnConfig "isNotBS3")) "nav-link")))
linkTo=(bs-default @linkToComponent (component "bs-link-to" class=(if (macroCondition (macroGetOwnConfig "isNotBS3")) "nav-link")))
dropdown=(component (bs-default @dropdownComponent (component "bs-dropdown")) inNav=true htmlTag="li")
)
}}

</ul>
2 changes: 1 addition & 1 deletion addon/components/bs-nav/item.js
Expand Up @@ -4,7 +4,7 @@ import { filter, filterBy, gt } from '@ember/object/computed';
import Component from '@ember/component';
import { action } from '@ember/object';
import { scheduleOnce } from '@ember/runloop';
import LinkComponent from '@ember/routing/link-component';
import LinkComponent from 'ember-bootstrap/components/bs-link-to';
import ComponentParent from 'ember-bootstrap/mixins/component-parent';
import overrideableCP from 'ember-bootstrap/utils/cp/overrideable';
import { assert } from '@ember/debug';
Expand Down
17 changes: 0 additions & 17 deletions addon/components/bs-nav/link-to.js

This file was deleted.

14 changes: 14 additions & 0 deletions addon/components/bs-navbar/link-to.hbs
@@ -0,0 +1,14 @@
<BsLinkTo
@route={{this.route}}
@model={{this.model}}
@models={{this.models}}
@query={{this.query}}
@replace={{@replace}}
@disabled={{@disabled}}
@current-when={{@current-when}}
@activeClass={{@activeClass}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think <BsLinkTo> supports @replace, @current-when and @activeClass. I guess a left-over from an earlier iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, will fix!

{{on "click" this.onClick}}
...attributes
>
{{yield}}
</BsLinkTo>
22 changes: 7 additions & 15 deletions addon/components/bs-navbar/link-to.js
@@ -1,6 +1,5 @@
import BsNavLinkToComponent from 'ember-bootstrap/components/bs-nav/link-to';
import defaultValue from 'ember-bootstrap/utils/default-decorator';

import Component from '@glimmer/component';
import { action } from '@ember/object';
/**
* Extended `{{link-to}}` component for use within Navbars.
*
Expand All @@ -9,25 +8,18 @@ import defaultValue from 'ember-bootstrap/utils/default-decorator';
* @extends Components.NavLinkTo
* @public
*/
export default class NavbarLinkTo extends BsNavLinkToComponent {
export default class NavbarLinkTo extends Component {
/**
* @property collapseNavbar
* @type {Boolean}
* @default true
* @public
*/
@defaultValue
collapseNavbar = true;

/**
* @event onCollapse
* @private
*/
onCollapse() {}

click() {
if (this.collapseNavbar) {
this.onCollapse();
@action
onClick() {
if (this.args.collapseNavbar ?? true) {
this.args.onCollapse();
}
}
}
1 change: 0 additions & 1 deletion app/components/bs-dropdown/menu/link-to.js

This file was deleted.

1 change: 1 addition & 0 deletions app/components/bs-link-to.js
@@ -0,0 +1 @@
export { default } from 'ember-bootstrap/components/bs-link-to';
1 change: 0 additions & 1 deletion app/components/bs-nav/link-to.js

This file was deleted.

25 changes: 10 additions & 15 deletions tests/acceptance/bs-nav-link-test.js
@@ -1,9 +1,6 @@
import { visit } from '@ember/test-helpers';
import { test, module } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import hasEmberVersion from '@ember/test-helpers/has-ember-version';

const supportsAngleBracketsLinkTo = hasEmberVersion(3, 10);

module('Acceptance | bs-nav-link', function (hooks) {
setupApplicationTest(hooks);
Expand All @@ -19,16 +16,14 @@ module('Acceptance | bs-nav-link', function (hooks) {
assert.dom(this.element.querySelectorAll('.nav li')[2]).hasClass('active');
});

if (supportsAngleBracketsLinkTo) {
test('active @route property marks nav item as active', async function (assert) {
await visit('/acceptance/linkto/1');
assert.dom(this.element.querySelectorAll('.nav li')[0]).hasClass('active');
assert.dom(this.element.querySelectorAll('.nav li')[1]).hasNoClass('active');
assert.dom(this.element.querySelectorAll('.nav li')[2]).hasClass('active');
await visit('/acceptance/linkto/2');
assert.dom(this.element.querySelectorAll('.nav li')[0]).hasNoClass('active');
assert.dom(this.element.querySelectorAll('.nav li')[1]).hasClass('active');
assert.dom(this.element.querySelectorAll('.nav li')[2]).hasClass('active');
});
}
test('active @route property marks nav item as active', async function (assert) {
await visit('/acceptance/linkto/1');
assert.dom(this.element.querySelectorAll('.nav li')[0]).hasClass('active');
assert.dom(this.element.querySelectorAll('.nav li')[1]).hasNoClass('active');
assert.dom(this.element.querySelectorAll('.nav li')[2]).hasClass('active');
await visit('/acceptance/linkto/2');
assert.dom(this.element.querySelectorAll('.nav li')[0]).hasNoClass('active');
assert.dom(this.element.querySelectorAll('.nav li')[1]).hasClass('active');
assert.dom(this.element.querySelectorAll('.nav li')[2]).hasClass('active');
});
});
2 changes: 0 additions & 2 deletions tests/helpers/setup-no-deprecations.js
Expand Up @@ -9,8 +9,6 @@ const ignoredDeprecations = [
/Versions of modifier manager capabilities prior to 3\.22 have been deprecated/,
/Usage of the Ember Global is deprecated./,
/import .* directly from/,
// this will be fixed in a future PR...
/Using Ember.LinkComponent/,
];

export default function setupNoDeprecations({ beforeEach, afterEach }) {
Expand Down