-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Router link component and routing helpers #339
Changes from 3 commits
b002ae7
820d3e4
0a7a550
29bbc4d
6b050b9
db59e8b
20f5570
f7da2c2
5c6beae
cc0d565
08e749d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
- Start Date: 2018-06-15 | ||
- RFC PR: | ||
- Ember Issue: | ||
|
||
# Add a Link component and routing helpers | ||
|
||
## Summary | ||
|
||
The purpose of this is to add a single component, `Link`, that wraps up some routing helpers for default usage of linking / routing. | ||
|
||
## Motivation | ||
|
||
Currently, on ember#canary, the Angle Bracket Invocation feature doesn't support positional arguments. For clarity, and with some inspiration from react-router, It may be beneficial for Ember to include a `Link` component, similar to the existing `LinkTo` (`link-to`), but with only named arguments. This will help with clarity of intent, and also allow for easier transitioning from other communities who already have named-argument links. | ||
|
||
## Detailed design | ||
|
||
Helpers to be implemented: | ||
- `transition` | ||
this is a helper that would tie into the router to be able to achive the same functionality as the simple usage of `LinkTo` | ||
|
||
example: | ||
|
||
``` | ||
<button {{action (transition to='posts.edit' postId=post.id}} | ||
``` | ||
|
||
- `replace-with` | ||
this is same as transition, but replaces the current URL and current history entry. This may be an argument to transition instead. e.g.: `(transition ... replace=true)` | ||
|
||
- `is-route-active` | ||
this returns a boolean representing whether or not the current route matches the passed argument. | ||
|
||
example: | ||
|
||
``` | ||
<button | ||
{{action (transition to='posts.edit' post=model.post)}} | ||
class={{if (is-route-active name='posts.edit' post=model.post) 'selected'}} /> | ||
``` | ||
|
||
this may need to support multiple invocation styles, for when there aren't parameters and the dev wants to be concise: | ||
|
||
example: | ||
|
||
``` | ||
<button ... class={{if (is-route-active 'posts') 'selected'}} /> | ||
``` | ||
|
||
Inspiration for helpers implementation could come from https://github.com/BBVAEngineering/ember-route-helpers | ||
|
||
Components to be implemented: | ||
- `Link` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that we can be more iterative here, and add the following named arguments to the existing
This would make transforming existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. desire to use the newer angle bracket invocation style was my original concern. but I couldn't come with up with named argument for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha! I was actually thinking exactly the opposite. Also, I think there is value in iterating forward within the component we all know / love / hate / etc. Adding a couple of named arguments to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol, guess the only way forward is with the smallest amount of changes. I'm totes good with named args in linked to. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would be 👍for making I actually looked at this when I was first playing with components. With a router.js (ignore that the route inheritance wouldn't really make sense
My ideal
I think this could actually be done completely backwards compatible while allowing positional params too (with deprecation later). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooo, @rtablada I like that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The names are a bit different and I would need to ask someone a bit more knowledgeable with the router to make named rtablada/link-to-params@5236ac5 The links I tested this with are: <LinkTo @params={{array "foo" 1 2}}>Positional Params</LinkTo>
<LinkTo @targetRouteName="foo" @models={{array 1 2}}>Named Params</LinkTo> There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, we'd probably want to alias |
||
|
||
`Link` could just be tagless component with the following template: | ||
```hbs | ||
{{!-- components/link/template.hbs --}} | ||
<a | ||
{{action (transition ...@to)}} | ||
class="{{if (is-route-active ...@to) 'active'}}" | ||
> | ||
|
||
{{yield}} | ||
|
||
</a> | ||
``` | ||
|
||
Usage: | ||
```hbs | ||
<Link @to=(array 'posts.edit' post)>Edit Post</Link> | ||
``` | ||
|
||
|
||
**Deprecation** `LinkTo` aka `link-to` | ||
|
||
The goal of `Link` and the route helpers is to provide a flexible way of routing from the template while providing a sample component with sensible defaults. This would achieve the exact same functionality as `LinkTo`, so `LinkTo` would no longer be needed and could eventually be removed. | ||
|
||
It's possible we could write a codemod to auto-convert everyone's non-angle-bracket invocation of `{{#link-to ...` to the new angle bracket component: `Link` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels more of an open question than a detailed design, due to the uncertainty of the proposal. |
||
|
||
## How we teach this | ||
|
||
We'll want to make it very clear that the traditional `LinkTo` technique of linking will still be available, but will eventually be deprecated. | ||
|
||
The documentation and guides would need to be incrementally upgraded to inform people about the new linking strategy -- but becaus the old way would still work, having some docs say `LinkTo` instead of `Link` wouldn't be the worst thing. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in "becaus" |
||
|
||
|
||
## Drawbacks | ||
|
||
The biggest drawback is that all routing documentation would be out of date, and there is a lot of routing documentation and blog posts throughout the web. | ||
|
||
Without positional params, the alternative may need to use the `array` helper for the `@to` argument... which could almost make people wonder why `LinkTo` didn't get a named argument for the route / route params. With the `Link` component, the arguments would read more ergonomically -- `<Link @to=...` vs `<LinkTo @route=...`. The latter implies that things other than routes could be linked to via the `LinkTo` component, which could cause confusion about the intended usage. | ||
|
||
## Alternatives | ||
|
||
There are two alternatives: | ||
|
||
1. Do nothing: | ||
- people will find that the `LinkTo` usage with angle bracket invocation to be somewhat awkward, maybe unintuitive. | ||
2. Only implement the routing helpers | ||
- people could define their own linking components however they desire | ||
|
||
## Unresolved questions | ||
|
||
- Is there a way for making `(array 'posts.edit' post)` more ergonomic? _having_ to use the array helper _seems_ like it shouldn't be a thing. Maybe if the `transition-to` helper took named arguments without `hash`? transition can be implemented to support this:`(transition to='posts.edit' postId=post.id)`.. but for `Link`, I don't know how the route arguments would get passed to `transition`. | ||
|
||
- For routes with multiple segment parameters, it may become hard to keep track of things. e.g.: `(array 'posts.edit.comments.reply' post comment.id)` how do I know where the parameters are going to go without looking at the router.js file? (though, this is an existing question) | ||
|
||
- Would it make sense to somehow have a way to statically confirm that the route path is valid? right now, it looks like a string -- how would I know if I typed it wrong? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Positional arguments are not planned for angle bracket invocations, I suggest updating this sentence to remove any "but in the future it might" implicit suggestion.