-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Closed
NullVoxPopuli
wants to merge
11
commits into
emberjs:master
from
NullVoxPopuli:router-link-and-helpers
Closed
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
b002ae7
Create 0000-router-link-and-helpers.md
NullVoxPopuli 820d3e4
Update 0000-router-link-and-helpers.md
NullVoxPopuli 0a7a550
Update 0000-router-link-and-helpers.md
NullVoxPopuli 29bbc4d
Fixed word spelling
Alonski 6b050b9
updates based on feedback
NullVoxPopuli db59e8b
formatting, update questions
NullVoxPopuli 20f5570
formatting
NullVoxPopuli f7da2c2
Update 0000-router-link-and-helpers.md
NullVoxPopuli 5c6beae
Update 0000-router-link-and-helpers.md
NullVoxPopuli cc0d565
Merge branch 'router-link-and-helpers' into patch-2
NullVoxPopuli 08e749d
Merge pull request #1 from Alonski/patch-2
NullVoxPopuli File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,124 @@ | ||
- 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 | ||
|
||
Positional arguments are not planned for use with Angle Bracket invocations. For clarity, and with some inspiration from react-router, It may be beneficial for Ember to include a `Link` component, similar to the existing `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 | ||
|
||
*Phase 1* | ||
- add built-in helpers for custom link-like behavior | ||
- add `Link` component with `@to` named argument -- all other named arguments will be the params | ||
|
||
*Phase 2* | ||
- deprecate `LinkTo` | ||
|
||
*Phase 3* | ||
- remove `LinkTo` and the corresponding deprecation | ||
|
||
### Details | ||
|
||
*Helpers to be implemented* | ||
|
||
- `transition` | ||
this is a helper that would tie into the router to be able to achieve the same functionality as the navigational usage of `link-to` | ||
|
||
examples: | ||
|
||
```hbs | ||
<button {{action (transition to='posts.edit' postId=post.id)}} /> | ||
Edit | ||
</button> | ||
|
||
<button {{action (transition to='login' replace=true)}} /> | ||
Login | ||
</button> | ||
``` | ||
|
||
|
||
|
||
- `is-route-active` | ||
this returns a boolean representing whether or not the current route matches the passed argument. | ||
|
||
example: | ||
|
||
```hbs | ||
<button | ||
{{action (transition to='posts.edit' post=model.post)}} | ||
class={{if (is-route-active '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: | ||
|
||
examples: | ||
|
||
```hbs | ||
<button ... class={{if (is-route-active 'posts') 'selected'}} /> | ||
No model params provided | ||
</button> | ||
``` | ||
|
||
|
||
*Add `Link` Component* | ||
|
||
`Link` could just be tagless component with the following template: | ||
|
||
```hbs | ||
{{!-- components/link/template.hbs --}} | ||
<a | ||
{{action (transition @to models=@models replace=@replace)}} | ||
...attributes | ||
class="{{if (is-route-active @to models=@models) 'active'}}"> | ||
|
||
{{yield}} | ||
|
||
</a> | ||
``` | ||
|
||
Usage: | ||
```hbs | ||
<Link @to='posts.edit' @models={{post}}>Edit Post</Link> | ||
``` | ||
|
||
where `@models` can be a `hash`, `array`, or just a single object or id. | ||
|
||
NullVoxPopuli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Starting with @rwjblue's [ember-router-helpers](https://github.com/rwjblue/ember-router-helpers) as a base, `is-route-active` is already implemented (as `is-active`, so this would need to be renamed), along with `route-params`, `transition-to`, and `url-for`. | ||
|
||
A [PR](https://github.com/rwjblue/ember-router-helpers/pull/46) has been started that upgrades `ember-router-helpers` and adds tests in preparation for implementing the helper renames and `Link` component. | ||
|
||
|
||
|
||
**Deprecation** `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 `link-to`, so `link-to` would no longer be needed and could eventually be removed. | ||
|
||
## How we teach this | ||
|
||
We'll want to make it very clear that the traditional `link-to` 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 because the old way would still work, having some docs say `link-to` instead of `Link` wouldn't be the worst thing. | ||
|
||
## 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 would feel awkward enough to make people wonder why `link-to` (as `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 `link-to` component, which could cause confusion about the intended usage. | ||
|
||
## Alternatives | ||
|
||
1. Only implement the routing helpers | ||
- people could define their own linking components however they desire | ||
|
||
## Inspiration / Code taken from | ||
- https://github.com/alexspeller/ember-cli-active-link-wrapper | ||
- https://github.com/BBVAEngineering/ember-route-helpers | ||
- https://github.com/peec/ember-transition-helper | ||
- https://github.com/rwjblue/ember-router-helpers |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is an accessibility red flag to me, links should be links. While the
transition
helper probably has value in other places I'd like to see an example that doesn't just turn a button into a styled link.