-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[DOC] Correctly mark public link-to component named arguments #19348
[DOC] Correctly mark public link-to component named arguments #19348
Conversation
@@ -406,6 +406,18 @@ const LinkComponent = EmberComponent.extend({ | |||
**/ | |||
replace: false, | |||
|
|||
/** | |||
Determines whether the `LinkComponent` will prevent the default |
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.
Based on the description of the Allowing Default Action
section in the LinkTo API docs.
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.
Now that I think about it, I wonder if this should just be deprecated and removed as part of RFC 707.
This seems only useful in the context of changing the tagName to something other than <a>
(which is deprecated in RFC 707).
When the tagName is <a>
, the default action of the browser is to navigate away, so there is no sense in not preventing it.
Thoughts?
Looping in @rwjblue as well.
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.
I'm not sure I completely understand what you mean. I've never used @preventDefault
myself, but I can imagine it being useful for some use cases to open a route in a new tab.
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.
Wow sorry, scratch that. I was confused. Yeah I can't immediately imagine a scenario where a refresh is desired.
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.
@chancancode - ya seems correct to me
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.
@chancancode - So... Merge now and update the RFC RE: deprecating preventDefault
?
@rwjblue @bertdeblock How about we move the docs currently in the "Allowing Default Action" section into the documentation for the property itself, and mark the property as Reading the docs again – I suppose there is case to be made for keeping this even if <LinkTo @route="dashboard" @preventDefault={{this.requiresRefresh}}>
Return to dashboard
</LinkTo> Where If someone wants to make the case for it, I think it wouldn't cost much to keep the functionality around. Personally, I think it's not really needed, and there are probably better ways of accomplishing that without using |
See discussion on emberjs/ember.js#19348
I opened a PR on the RFC repo to amend this if we want to go forward: emberjs/rfcs#710 |
This sounds good to me. I don't have any incentive to keep this functionality around, but of course maybe other people do. Let me know when / if I should update this PR to move the docs 👍. |
I think you can go ahead, if we ended up wanting to make it public it's easy enough to change the tag. |
Moved the docs (hopefully I understood it correctly). Let me know if anything. |
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.
@rwjblue seems good to you?
Gentle ping. Anything left I should take care of? |
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.
Sorry for the delay!
Based on this RFC summary and this part of the API docs.
cc @chancancode.