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

[native] Add 'component' prop to DOM 'Link' #4625

Closed
necolas opened this issue Mar 3, 2017 · 17 comments
Closed

[native] Add 'component' prop to DOM 'Link' #4625

necolas opened this issue Mar 3, 2017 · 17 comments
Labels

Comments

@necolas
Copy link

necolas commented Mar 3, 2017

The native router'sLink component has a different props interface to the DOM package Link. It would be good to align them a little closer, for example, supporting the component prop for the DOM variant. This would a allow people to more easily use the router with something like react-native-web

@timdorr
Copy link
Member

timdorr commented Mar 3, 2017

Can you not use react-router-native's Link on react-native-web?

@necolas
Copy link
Author

necolas commented Mar 3, 2017

You still need a DOM-based router. We'd use this on mobile.twitter.com too

@ryanflorence
Copy link
Member

ryanflorence commented Mar 8, 2017

We get this request often but I'm not sure anything will ever convince me to support it.

On the web an <a href/> means something. When you command/control click it, you get a new window, or right click and choose "open in new tab". Screen readers treat them differently, etc.

If we supported <Link component="div"/> not only would we be spitting out invalid markup (<div href/>) we'd also be completely breaking accessibility. To properly turn a non-anchor into a link for a screenreaders you need to

  • add the role="link" attribute
  • add the tabindex="0" attribute
  • register keydown event
    • check in keydown if its the Enter key, if so, navigate
  • register a keyup event
    • check if the keyup is the Space key, if so, navigate

The point of Link in the dom package is to create accessible links that work the way links are supposed to work, not give people a footgun and break how browsers work for all users, not just screen readers. Also, we'd be generating invalid HTML while we were at it.

Now ... that said, your use-case is the first one where I'm like "oh, that's interesting." Any ideas on how to support your goals w/o giving people a footgun?

@ryanflorence
Copy link
Member

ryanflorence commented Mar 8, 2017

oh hey ... check this out:

componentDidMount() {
  warning(findDOMNode(this).tagName !== 'A', 'YOU BLEW IT!')
}

s/YOU BLEW IT!/[some actual message about needing to render an anchor]/

@megamaddu
Copy link

I'm not familiar with React Native so I'm curious if this is different -- on web if you want your a to have something other than text inside it that's fine, and ends up working exactly the same in React with react-router's Link:

<Link to="kittens.com">
  <img ... />
</Link>

or:

<Link to="/about">
  <FancyFlexLayoutMenuItemWithDancingIcons>About</...>
</Link>

I've never run into a situation where my ability to style or use a Link depended on bypassing the a wrapper.

@necolas
Copy link
Author

necolas commented Mar 8, 2017

Link has a few limitations in the DOM package and they are exaggerated when you're using a library with richer primitives than the DOM provides. For React Native: The default layout doesn't match View or Text; the style interface is incompatible in multiple ways; it's missing propTypes like accessibilityLabel; it doesn't support callbacks like onLayout or onPress*. You can't really work around that in your app either, so you end up copy pasting the internals and making your own Link component/package. Doing so also has the benefit of allowing you to publish components that contain links and work with a certain router context, but don't depend explicitly on RR.

I figured there would be less interest in creating a router-agnostic Link package (haven't looked fully into how Next.js avoids needing a link component, but that's interesting too).

It's certainly a set of complicated trade offs. Warning if the base tag is not an anchor sounds like a reasonable approach. I can get back to you with some specific examples of what we do or want to do

@mjackson
Copy link
Member

mjackson commented Mar 8, 2017

@ryanflorence +1 for warning about using something other than a <a>. I'd like to figure out a way to do it w/out using findDOMNode thought.

@necolas I'd be grateful if you could take a look at our native <Link> and fill in the missing pieces. Our current implementation is pretty basic, but enough to demonstrate that it works.

@ryanflorence
Copy link
Member

ryanflorence commented Mar 8, 2017

@mjackson you have refs and you have findDOMNode. I don't know how to use refs to get to the dom node when the component passed in can be several levels of components before getting to the a.

On another note, we don't even know if our props (href, onClick) will make it all the way down.

const Thing = () => <Stuff/>
const Stuff = () => <Other/>
const Other = () => <a onClick={noRouterClick}/>

<Link component={Thing}/> // :\

How do we:

  1. Get a handle on that final tag to check if it's an a? (findDOMNode is the only thing I know of)
  2. How do we make sure out click handler makes it down there? (I don't think we can)

@ryanflorence
Copy link
Member

I'd be grateful if you could take a look at our native and fill in the missing pieces.

It's not the native Link he wants changed. The native Link already supports the component prop because there is no platform equivalent to <a>. There are TouchableHighlight, TouchableOpacity, and TouchableWithoutFeedback. TouchableHighlight is the default, but you can pass in the one you want.

What he's asking for is for the dom Link to support it so that he can run the same "native code" codebase on the web. But since the platform semantics around Link vary, I struggle to see how we can accommodate it w/o breaking the platform expectations.

Can't this be solved in react-native-web?

@mjackson
Copy link
Member

mjackson commented Mar 8, 2017

@ryanflorence When @necolas said:

For React Native: The default layout doesn't match View or Text; the style interface is incompatible in multiple ways; it's missing propTypes like accessibilityLabel; it doesn't support callbacks like onLayout or onPress*.

I thought he was critiquing our current native <Link> impl. Maybe I misunderstood?

@ryanflorence
Copy link
Member

Oh, sorry, I was reading to hastily :(

Yeah, we need major help on the Link component there in react native!

@necolas
Copy link
Author

necolas commented Mar 9, 2017

To clarify: I meant React Native for Web. If you want to run your RN components (that use RR) on the web, you can't because the Link API and layout is different depending on platform. Coming from the other direction: if you want to write your web app itself using RNW, you have a bad time because of the problems I mentioned.

I'll take a look at what an implementation of Link might look like if it's built with the RNW API.

@ryanflorence ryanflorence changed the title Add 'component' prop to DOM 'Link' [native] Add 'component' prop to DOM 'Link' Mar 10, 2017
@lamflam
Copy link

lamflam commented Feb 12, 2018

I found this issue while searching for a solution to another valid use case. We use glamorous for theming/styling. With glamorous, to get an anchor tag with appropriate styles we do something like:

const Anchor = glamorous.a(/* ...style code */)

to get a styled component that will still render <a></a> in the DOM. Since there is currently no way to pass this Anchor component to Link we lose all of this styling.

I understand the push for verifying that what gets rendered is in fact an anchor tag, but it would be great to allow some type of customization here.

@timdorr
Copy link
Member

timdorr commented Feb 12, 2018

@lamflam You can do that with glamorous's composition:

const Anchor = glamorous(Link)({ /* ...styles */ })

@necolas
Copy link
Author

necolas commented Feb 12, 2018

Closing due to inactivity. I still think the Link component API could be more platform-independent. But you can make your own Link and use that instead of what is in RR

@necolas necolas closed this as completed Feb 12, 2018
@lamflam
Copy link

lamflam commented Feb 12, 2018

@timdorr Can't believe I missed that, but thank you for pointing it out, works great!

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018
@mjackson
Copy link
Member

Just following up here: I think making the react-router-dom API consistent with the react-router-native API would actually be really nice. The same issue was brought up in #5437, which I've re-opened. Let's follow up with this discussion there, @necolas.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants