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] Further cleanup of the link-to component. #12198

Merged
merged 1 commit into from
Sep 5, 2015

Conversation

nathanhammond
Copy link
Member

  • Make it easier to extend by pulling more functionality into scope.
  • Removed unnecessarily imperative code.
  • Combine the loops for the params model deprecation search.
  • Make what's happening with params more explicit with comments.
  • Name anonymous functions.
  • Reword and polish documentation.
  • var => let

@nathanhammond
Copy link
Member Author

Notes:

  • I didn't change documentation links, but seeing as this is no longer a Handlebars helper we might should do that at this time.
  • Should I make computeActive and isActiveForRoute accessible? Or inline them?
  • Likely ghost-in-the-machine type failure from Travis.

Next things I want to do:

  • Deprecate inline link-to (and at the same time deprecate triple curly link-to) for 3.0.
  • Investigate @ef4's work to see what else I should do (as a separate PR since that feels more like a feature).

@mixonic
Copy link
Member

mixonic commented Aug 25, 2015

documentation-wise, I agree this is fine to leave on Ember.Templates.helpers. We treat pretty much all our components (like input) as helpers in the docs. Perhaps Ember.Templates.helpers should change, but I'm kind of liking it as a stable home. There is, in fact, no actual variable of that name. Its presence simply satisfies yuidoc.

@@ -334,7 +334,7 @@ linkToTemplate.meta.revision = 'Ember@VERSION_STRING_PLACEHOLDER';
a supplied route by name.

Instances of `LinkComponent` will most likely be created through
the `link-to` Handlebars helper, but properties of this class
Copy link
Member

Choose a reason for hiding this comment

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

This seems weird? I don't think we use the phrase "component tag" to describe {{link-to anywhere else. I might just reword all this.

Ember.LinkComponent components are invoked with {{#link-to}}. Properties
of this class can be overridden with reopen to customize application-wide behavior.

Also change @see below this to Ember.Templates.helpers.link-to

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed.

@rwjblue
Copy link
Member

rwjblue commented Aug 25, 2015

Deprecate inline link-to (and at the same time deprecate triple curly link-to) for 3.0.

IMHO, these are two separate issues:

  • deprecating triple curly link-to (which will require the -html-safe helper to become public and likely change names to unsafe-html as suggested by @mmun). This can be done at any time (exposing unsafe-html will be a feature flagged thing though).
  • deprecate inline link-to. As discussed previously this will receive the most pushback so it should be completely separate from actual feature work to be done.

@ef4
Copy link
Contributor

ef4 commented Aug 25, 2015

Hey @nathanhammond, nice work. My previous attempt is at master...ef4:link-to-cleanup. I stopped when I realized I had no public API for dealing with the triple curly case, I am happy to see it on the road to deprecation.

One of my goals is to implement link-to on top of helpers that are themselves reusable, like:

<a href={{url-for "posts"}} class={{if (route-is-active "posts") "active"}}>Posts</a>

That would be much nicer than having people extend LinkComponent. People who want customized link behavior throughout their app can just create a standard component using these helpers, without learning any special API.

A minor suggestion would be to make the helpers accept the same path & models arguments that link-to does, for ease of use, but also accept a list of models passed explicitly to handle the case where it is variable length. So both:

{{url-for "person" thePerson}}
{{url-for nextRoute models=nextRouteModels}}

@rwjblue
Copy link
Member

rwjblue commented Aug 25, 2015

I am happy to see it on the road to deprecation

@ef4 - We (actually @mmun) handled it with an AST transform for now.

- Make it easier to extend by pulling more functionality into scope.
- Removed unnecessarily imperative code.
- Combine the loops for the params model deprecation search.
- Make what's happening with params more explicit with comments.
- Name anonymous functions.
- Reword and polish documentation.
- `var` => `let`
@nathanhammond
Copy link
Member Author

This PR is set to land if everybody is okay with what I've done here. Sauce is just being flaky. Note that I've finished moving all functionality into the component and out of the hidden module scope, so re-review code. :)

@rwjblue I agree that those are two separate issues, especially since I now know how they're implemented. :)

@ef4 The other trick for a composable approach is making sure that _invoke is properly handled–exposing it to almost work will likely drive some people batty (not knowing that they need to have an action handle the transition).

I'll open additional PRs as I have time to:

  • Deprecate triple-curly link-to. This probably isn't contentious.
  • Deprecate inline link-to. I suspect that this will be contentious, but it's easy enough to deprecate that we can discuss over a concrete implementation in a PR.
  • Make LinkComponent a GlimmerComponent.
  • Move some logic into helpers a la @ef4 to allow for easier external construction.

@mmun
Copy link
Member

mmun commented Aug 26, 2015

We can amend that transform to handle the entire inline case (by converting it to block form) so that you only have to worry about the block form in your implementation.

@nathanhammond
Copy link
Member Author

Can we review and land this PR? I want to start with it as a base for more work on this today.

@ef4 @rwjblue @mmun

@rwjblue
Copy link
Member

rwjblue commented Sep 3, 2015

Seems good to me. I restarted the sauce labs job.

rwjblue added a commit that referenced this pull request Sep 5, 2015
[BUGFIX beta] Further cleanup of the link-to component.
@rwjblue rwjblue merged commit c0708b7 into emberjs:master Sep 5, 2015
@cibernox
Copy link
Contributor

cibernox commented Sep 6, 2015

I just chime in because I saw mentioned the intention to deprecate inline link-to and I was confused if that refers to the blockless {{link-to}}

@rwjblue
Copy link
Member

rwjblue commented Sep 6, 2015

Yes, inline link-to refers to the {{link-to 'link title' 'route name'}} form.

@cibernox
Copy link
Contributor

cibernox commented Sep 6, 2015

Shame, I do like that syntax. What's the reason, out of curiosity? Simplify the component?

@rwjblue
Copy link
Member

rwjblue commented Sep 6, 2015

@cibernox - I personally do not feel strongly about deprecating and as mentioned above, I believe deprecating will have pushback. The main motivation would be complications in the component, but I think that #12229 completely allows us to remove any code in Ember.LinkComponent supporting inline form.

@cibernox
Copy link
Contributor

cibernox commented Sep 6, 2015

@rwjblue Nice approach to do as much work in compile time to make the component simpler.

@rwjblue
Copy link
Member

rwjblue commented Sep 6, 2015

Ya, agreed. I personally have no issues with inline link-to once that lands...

@nathanhammond
Copy link
Member Author

@rwjblue is correct. Supporting the positional parameter for the inline link-to has weird consequences on the way the component has to be built (we're using a symbol to protect ourselves from that), and also things like {{{link-to "<b>this is bold</b>" "index"}}}.

I was proposing the deprecation before @mmun mentioned that we could do it at compile time. Also, once I finish adapting @ef4's helper-based link-to it'll be possible to implement a component that handles linking without extending Ember.LinkComponent which should make it possible to recreate the inline behavior.

I'm still not a fan of {{link-to}} as to the best of my knowledge it's the only place in the entire library where we use positional parameters to pass information into a component, but that's more of a pedagogical concern now with @mmun's solution. (More exceptions to the rules === more things you have to learn.)

All that said, I still want to deprecate {{{link-to}}} as that feels especially bad.

@rwjblue
Copy link
Member

rwjblue commented Sep 6, 2015

I still want to deprecate {{{link-to}}} as that feels especially bad.

No arguments here.

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.

6 participants