-
-
Notifications
You must be signed in to change notification settings - Fork 143
Conversation
Awesome! Could we do a little research on other single page app link components on GitHub and see if we are missing any other logic? We really want this link to look and feel like a link. This article describes this a bit more: http://jgaskins.org/blog/2016/07/30/don-t-change-perceived-browser-functionality. It would be great to check out the source code of a popular component on GitHub that has gone through all of these different edge cases: 😸 From http://jgaskins.org/blog/2016/07/30/don-t-change-perceived-browser-functionality, they recommend this:
|
That's a good point Chris. Added the other modifiers for now, I'll look for another similar component and see if we're missing anything else that might be nice to have. |
Handling "enter" on focussed elements (you can "select" links by "tabbing" through the page) might be another one to consider. We might already handle this one effectively though. |
Added |
Added in the Closes #770 . |
src/components/Link.react.js
Outdated
@@ -81,6 +87,7 @@ export default class Link extends Component { | |||
href={href} | |||
onClick={e => this.updateLocation(e)} | |||
title={title} | |||
target={external ? '_blank' : '_self'} |
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 think that I'd prefer that we just stick to the HTML convention here instead of defining a new API. That way, this component is more or less 1-1 with html.A
. So, just let users pass through target
directly rather than defining a new external
property.
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've seen a couple of other link components define external as a separate property, but I think you raise a good point, for both maintaining parity with html.A
and offering a bit more freedom for the user to decide what to target.
Changed in 6514c83.
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 think that I'd prefer that we just stick to the HTML convention here
I agree @chriddyp - cc @Marc-Andre-Rivet - we had a similar discussion a couple of days ago, also because there are other values of target
that still matter.
ctrlKey
conditionalThere 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 PR addresses #748 and adds the ability to control+click a
Link
to open it in a new tab. Althoughhtml.A
is a better component to use for external links, this adds that functionality for people who might use it within a multi-page Dash app.This PR also adds in the
target
prop to add logic that one would expect from a traditional link.Closes #748 .
Closes #770