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.

Sign up for GitHub

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

Jump to bottom

Remove LinkTo's tagName #19662

Closed
wants to merge 2 commits into from
Closed

Remove LinkTo's tagName #19662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f074d3
Remove LinkTo's tagName
nlfurniss Jul 19, 2021
827c35a
maybe solution to tagName
nlfurniss Jul 20, 2021
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
30 changes: 0 additions & 30 deletions packages/@ember/-internals/glimmer/lib/components/-link-to.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,27 +201,6 @@ import layout from '../templates/-link-to';
When transitioning into the linked route, the `model` hook will
be triggered with parameters including this passed identifier.

### Supplying a `tagName`

By default `<LinkTo>` renders an `<a>` element. This can be overridden for a single use of
`<LinkTo>` by supplying a `tagName` argument:

```handlebars
<LinkTo @route='photoGallery' @tagName='li'>
Great Hamster Photos
</LinkTo>
```

This produces:

```html
<li>
Great Hamster Photos
</li>
```

In general, this is not recommended.

### Supplying query parameters

If you need to add optional key-value pairs that appear to the right of the ? in a URL,
Expand Down Expand Up @@ -791,9 +770,6 @@ const LinkComponent = EmberComponent.extend({
Sets the element's `href` attribute to the url for
the `LinkComponent`'s targeted route.

If the `LinkComponent`'s `tagName` is changed to a value other
than `a`, this property will be ignored.

@property href
@private
*/
Expand All @@ -802,14 +778,9 @@ const LinkComponent = EmberComponent.extend({
'_route',
'_models',
'_query',
'tagName',
'loading',
'loadingHref',
function computeLinkToComponentHref(this: any) {
if (this.tagName !== 'a') {
return;
}

if (this.loading) {
return this.loadingHref;
}
Expand Down Expand Up @@ -869,7 +840,6 @@ const LinkComponent = EmberComponent.extend({

/**
The default href value to use while a link-to is loading.
Only applies when tagName is 'a'

@property loadingHref
@type String
Expand Down
39 changes: 5 additions & 34 deletions packages/@ember/-internals/glimmer/lib/components/link-to.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ class LinkTo extends InternalComponent implements DeprecatingInternalComponent {
!('model' in this.args.named && 'models' in this.args.named)
);

assert(
'Passing the `@tagName` argument to <LinkTo> is not supported.',
!('tagName' in this.args.named)
);
Comment on lines +83 to +86
Copy link
Member

Choose a reason for hiding this comment

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

Two concerns:

With this implemented as an assert, what does that mean for production? Would a @tagName be applied in production, even if it the assert stopped you in development? Seems bad if so. Maybe it should be a throw.

But, further, IMO we don't need a check at all. I don't think we're planning to add asserts for every old component argument which was removed in 4.0. They were removed, which means passing one of those arguments (like @tagName) could just mean it is ignored. I can ask around on this second point, but the first concern about production behavior not applying the tag name needs to be confirmed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, makes sense re: assert. As to the second point, classic components will keep tagName, so it seems like something special will need to be done for linkTo.


super.validateArguments();
}

Expand Down Expand Up @@ -418,40 +423,6 @@ if (EMBER_MODERNIZED_BUILT_IN_COMPONENTS) {
});
}

// @tagName
{
let superOnUnsupportedArgument = prototype['onUnsupportedArgument'];

Object.defineProperty(prototype, 'onUnsupportedArgument', {
configurable: true,
enumerable: false,
value: function onUnsupportedArgument(this: LinkTo, name: string): void {
if (name === 'tagName') {
let tagName = this.named('tagName');

deprecate(
`Passing the \`@tagName\` argument to <LinkTo> is deprecated. Using a <${tagName}> ` +
'element for navigation is not recommended as it creates issues with assistive ' +
'technologies. Remove this argument to use the default <a> element. In the rare ' +
'cases that calls for using a different element, refactor to use the router ' +
'service inside a custom event handler instead.',
false,
{
id: 'ember.link-to.tag-name',
for: 'ember-source',
since: {},
until: '4.0.0',
}
);

this.modernized = false;
Copy link
Member

Choose a reason for hiding this comment

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

It is asserted that passing <LinkTo @tagName="div"> still renders an <a> after this removal?

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 call. I added an assert to the validateArguments method and some tests that assert for this error, but not sure if that's the right place to throw versus where @href throws

} else {
superOnUnsupportedArgument.call(this, name);
}
},
});
}

// @bubbles & @preventDefault
{
let superIsSupportedArgument = prototype['isSupportedArgument'];
Expand Down
4 changes: 1 addition & 3 deletions ...@ember/-internals/glimmer/tests/integration/components/link-to/query-params-angle-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,12 +351,10 @@ moduleFor(
}

async ['@test supplied QP properties can be bound in legacy components'](assert) {
expectDeprecation(/Passing the `@tagName` argument to/);

this.addTemplate(
'index',
`
<LinkTo @tagName="a" id="the-link" @query={{hash foo=this.boundThing}}>
<LinkTo id="the-link" @query={{hash foo=this.boundThing}}>
Index
</LinkTo>
`
Expand Down
6 changes: 6 additions & 0 deletions ...es/@ember/-internals/glimmer/tests/integration/components/link-to/rendering-angle-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,12 @@ moduleFor(
content: 'Go to Index',
});
}

['@test it should throw an error if `tagName` is passed in']() {
expectAssertion(() => {
this.render(`<LinkTo @route='index' @tagName='button'>Go to Index</LinkTo>`);
}, /Passing the `@tagName` argument to <LinkTo> is not supported./);
}
}
);

Expand Down
6 changes: 6 additions & 0 deletions ...es/@ember/-internals/glimmer/tests/integration/components/link-to/rendering-curly-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,5 +192,11 @@ moduleFor(

this.assertText('Go to Index');
}

['@test it should throw an error if `tagName` is passed in']() {
expectAssertion(() => {
this.render(`{{#link-to route='index' tagName='button'}}Go to Index{{/link-to}}`);
}, /Passing the `@tagName` argument to <LinkTo> is not supported./);
}
}
);
14 changes: 0 additions & 14 deletions ...ages/@ember/-internals/glimmer/tests/integration/components/link-to/routing-angle-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,20 +133,6 @@ moduleFor(
);
}

async [`@test [DEPRECATED] it doesn't add an href when the tagName isn't 'a'`](assert) {
this.addTemplate(
'index',
`<LinkTo @route='about' id='about-link' @tagName='div'>About</LinkTo>`
);

await expectDeprecationAsync(
() => this.visit('/'),
/Passing the `@tagName` argument to <LinkTo> is deprecated\./,
EMBER_MODERNIZED_BUILT_IN_COMPONENTS
);
assert.strictEqual(this.$('#about-link').attr('href'), null, 'there is no href attribute');
}

async [`@test it applies a 'disabled' class when disabled`](assert) {
this.addTemplate(
'index',
Expand Down
19 changes: 0 additions & 19 deletions ...ages/@ember/-internals/glimmer/tests/integration/components/link-to/routing-curly-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,25 +133,6 @@ moduleFor(
);
}

async [`@test [DEPRECATED] it doesn't add an href when the tagName isn't 'a'`](assert) {
this.addTemplate(
'index',
`<div id='about-link'>{{#link-to route='about' tagName='div'}}About{{/link-to}}</div>`
);

await expectDeprecationAsync(
() => this.visit('/'),
/Passing the `@tagName` argument to <LinkTo> is deprecated\./,
EMBER_MODERNIZED_BUILT_IN_COMPONENTS
);

assert.strictEqual(
this.$('#about-link > div').attr('href'),
null,
'there is no href attribute'
);
}

async [`@test it applies a 'disabled' class when disabled`](assert) {
this.addTemplate(
'index',
Expand Down
13 changes: 4 additions & 9 deletions ...nternals/glimmer/tests/integration/components/link-to/transitioning-classes-angle-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { RSVP } from '@ember/-internals/runtime';
import { Route } from '@ember/-internals/routing';
import { EMBER_MODERNIZED_BUILT_IN_COMPONENTS } from '@ember/canary-features';
import { moduleFor, ApplicationTestCase, runTask } from 'internal-test-helpers';

function assertHasClass(assert, selector, label) {
Expand Down Expand Up @@ -190,25 +189,21 @@ moduleFor(
'application',
`
{{outlet}}
<LinkTo @tagName='li' @route='index'>
<LinkTo @route='index'>
<LinkTo id='index-link' @route='index'>Index</LinkTo>
</LinkTo>
<LinkTo @tagName='li' @route='parent-route.about'>
<LinkTo @route='parent-route.about'>
<LinkTo id='about-link' @route='parent-route.about'>About</LinkTo>
</LinkTo>
<LinkTo @tagName='li' @route='parent-route.other'>
<LinkTo @route='parent-route.other'>
<LinkTo id='other-link' @route='parent-route.other'>Other</LinkTo>
</LinkTo>
`
);
}

async beforeEach() {
return expectDeprecationAsync(
() => this.visit('/'),
/Passing the `@tagName` argument to <LinkTo> is deprecated\./,
EMBER_MODERNIZED_BUILT_IN_COMPONENTS
);
return this.visit('/');
}

resolveAbout() {
Expand Down
13 changes: 4 additions & 9 deletions ...nternals/glimmer/tests/integration/components/link-to/transitioning-classes-curly-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { RSVP } from '@ember/-internals/runtime';
import { Route } from '@ember/-internals/routing';
import { EMBER_MODERNIZED_BUILT_IN_COMPONENTS } from '@ember/canary-features';
import { moduleFor, ApplicationTestCase, runTask } from 'internal-test-helpers';

function assertHasClass(assert, selector, label) {
Expand Down Expand Up @@ -190,25 +189,21 @@ moduleFor(
'application',
`
{{outlet}}
{{#link-to route='index' tagName='li'}}
{{#link-to route='index'}}
<div id='index-link'>{{#link-to route='index'}}Index{{/link-to}}</div>
{{/link-to}}
{{#link-to route='parent-route.about' tagName='li'}}
{{#link-to route='parent-route.about'}}
<div id='about-link'>{{#link-to route='parent-route.about'}}About{{/link-to}}</div>
{{/link-to}}
{{#link-to route='parent-route.other' tagName='li'}}
{{#link-to route='parent-route.other'}}
<div id='other-link'>{{#link-to route='parent-route.other'}}Other{{/link-to}}</div>
{{/link-to}}
`
);
}

async beforeEach() {
return expectDeprecationAsync(
() => this.visit('/'),
/Passing the `@tagName` argument to <LinkTo> is deprecated\./,
EMBER_MODERNIZED_BUILT_IN_COMPONENTS
);
return this.visit('/');
}

resolveAbout() {
Expand Down