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] Partially revert #12229 and add a test for #13432 #13438

Merged
merged 1 commit into from
May 2, 2016

Conversation

Serabe
Copy link
Member

@Serabe Serabe commented May 2, 2016

This PR partially reverts #12229 given that it made the inline form of
components extending from LinkTo impossible.

Beware that matching the exact behaviour is not there yet (as it wasn't
before #12229) given that {{{link-to title route}}} would unescape title
while {{{my-link-to title route}}} would not. This behaviour was not working
before and therefore is not working after this revert.

This PR does not address whether the inline form for link-to should be
deprecated or not.

@Serabe
Copy link
Member Author

Serabe commented May 2, 2016

OTOH, I think the behaviour for triple curlies in link-to should be deprecated given:

  • It is a hack not easily replicable in the component itself.
  • Makes link-to behave differently from other components.
  • That behaviour cannot be ported easily to all components extending from link-to.
  • It can be easily transformed from {{{link-to title route}}} to {{#link-to route}} {{{title}}} {{/link-to}}.

@rwjblue
Copy link
Member

rwjblue commented May 2, 2016

Ultimately, I'd like to deprecate this inline form for custom Ember.LinkComponent classes (and possibly even for all inline link-to), so leaving the AST transform for {{link-to 'thing' 'thing'}} but bringing back support in packages/ember-routing-views/lib/components/link-to.js allows us to support the old behavior and still deprecate custom LinkComponent inline form.

tldr; Leave the AST transform added in #12229 in, add the test you have added here, and revert the changes #12229 made to packages/ember-routing-views/lib/components/link-to.js.

@Serabe Serabe changed the title [BUGFIX beta] Revert #12229 and add a test for #13432 [BUGFIX beta] Partially revert #12229 and add a test for #13432 May 2, 2016
@Serabe
Copy link
Member Author

Serabe commented May 2, 2016

Updated! Thank you for the quick response!

@rwjblue
Copy link
Member

rwjblue commented May 2, 2016

Awesome, this LGTM. We are having an issue with Sauce Labs ATM (they aren't recognizing one of the Firefox builds). I'll try to dig into that later today.

…js#13432

This PR partially reverts emberjs#12229 given that it made the inline form of
components extending from `LinkTo` impossible.

Beware that matching the exact behaviour is not there yet (as it wasn't
before emberjs#12229) given that {{{link-to title route}}} would unescape title
while {{{my-link-to title route}}} would not. This behaviour was not working
before and therefore is not working after this revert.

This PR does not address whether the inline form for link-to should be
deprecated or not.
@rwjblue
Copy link
Member

rwjblue commented May 2, 2016

#13439 should have fixed the build, thanks for rebasing so quickly

@rwjblue rwjblue merged commit 9c6d2d8 into emberjs:master May 2, 2016
@rwjblue
Copy link
Member

rwjblue commented May 2, 2016

Thanks @Serabe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants