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

Make the to prop optional #2319

Closed
bottleofwater opened this issue Oct 20, 2015 · 23 comments
Closed

Make the to prop optional #2319

bottleofwater opened this issue Oct 20, 2015 · 23 comments
Labels

Comments

@bottleofwater
Copy link

Could it be possible to make the to prop optional on the Link component (if missing, nothing would happen, just like a <a/> tag without its href attribute)? It would be useful when writing generic components, that may or may not be links.

@bottleofwater
Copy link
Author

For what it's worth, a workaround is to set up a wrapper :

export class OptionalLink extends React.Component {

    render( ) {

        return this.props.to
            ? <ReactRouter.Link {... this.props} />
            : <a { ... this.props} />;

    }

};

Not so pretty, though :(

@taion
Copy link
Contributor

taion commented Oct 20, 2015

Those aren't equivalent - Link uses an <a>, not a <div>. In general since Link is intended as a drop-in for an anchor, and anchors don't let you do that, I don't think this would be a worthwhile feature to add.

@taion taion closed this as completed Oct 20, 2015
@bottleofwater
Copy link
Author

@taion If you mean "anchor" as a synonym for a <a> tag, then I believe you're in the wrong.
Quoting the html5 spec itself:

If the a element has no href attribute, then the element represents a placeholder for where a link might otherwise have been placed, if it had been relevant, consisting of just the element's contents.

So currently, <Link> is not a valid drop-in replacement for <a>.

@taion taion reopened this Oct 20, 2015
@taion
Copy link
Contributor

taion commented Oct 20, 2015

Interesting. Okay.

@knowbody
Copy link
Contributor

quick hack:

<Link to=' '>Nothing</Link>

@knowbody
Copy link
Contributor

but to be honest I don't see the point of having that

@taion
Copy link
Contributor

taion commented Oct 20, 2015

It does seem a little pointless. Maybe this can be shelved for whatever pending re-write of Link that's supposedly coming in the future?

@knowbody
Copy link
Contributor

please give me a use case for using an empty Link

@bottleofwater
Copy link
Author

Well, any time you may or may not have a link, which is quite often if you write generic components (as we could find in libraries). For example, in my case, I have a grid component. Each cell may or may not be a link, depending on its content. In the current state, I have to either duplicate my code to handle the splitted is-a-link / is-not-a-link logic, or wrap the Link component to add this feature myself (which is what I did, with the same code that the one in the second comment).

Frankly, giving the Link source code a cursory read, I feel like that supporting this use case wouldn't add any sort of complexity (just an early return with <a {...props} />, should the to prop be falsy), so if you haven't got the time to work on this, I can probably do it myself and submit a PR. What do you think?

@knowbody
Copy link
Contributor

I still don't think this is something we should add to the API. There are ways to accomplish it, you could use onClick={(e) => e.preventDefault} if the 'to' path is not passed to your component. Thoughts @taion @mjackson?

@bottleofwater
Copy link
Author

Sure, there's others ways to do it (mostly hacks, as you said in your first post). But I already know how to get this behaviour if I want to. I see this issue more as a discussion over the behaviour that should be expected of a Link without a to prop than an help request. And my opinion is that it should follow the behaviour of the <a> tag.

@knowbody
Copy link
Contributor

I get your point, let's see what others think about it, I'm happy to add it, just want to see other opinions 😄

This is what we are looking at: http://www.w3.org/TR/html-markup/a.html#placeholder-hyperlink

@taion
Copy link
Contributor

taion commented Oct 21, 2015

My 2 cents -

  • As a primary maintainer on one of the more popular React component libraries, we've never hit the "empty href" case for link-like components, and in fact don't even support it: https://github.com/react-bootstrap/react-bootstrap/blob/v0.27.2/src/SafeAnchor.js#L27
  • In practice, if you want a link-like thing to be generic, presumably you'd have to support both React Router <Link> for intra-router navigation and just standard <a> for external navigation anyway, so you'd need to branch anyway for a generic link
  • The a11y for this sort of placeholder seems not ideal - at any rate you're still styling it as a link despite its not being a link

So thinking about this a bit more, I'd be against adding this to React Router.

@knowbody
Copy link
Contributor

I think the "2 cents" from @taion are the strong points, closing.

@mjackson @ryanflorence if you think that's something we should include, please reopen.

@macgyver
Copy link

macgyver commented Aug 15, 2016

The a11y for this sort of placeholder seems not ideal - at any rate you're still styling it as a link despite its not being a link

Styling links with hrefs vs links w/o is trivial:

a {
  // styles for all links, including placeholder links
}

a[href] {
  // styles for only links that actually have hrefs
}

Indeed browser default (user-agent) stylesheets already make a visual distinction. Assistive Technology can handle a link with no href, and browsers like Chrome and Firefox are not inaccessible out-of-the-box.

In practice, if you want a link-like thing to be generic, presumably you'd have to support both React Router for intra-router navigation and just standard for external navigation anyway, so you'd need to branch anyway for a generic link

I haven't used the React Router enough to comment on the ergonomics of external links, but as a web author I've come across a few scenarios when I knew a thing ultimately would link somewhere, but didn't have a valid URL at the exact moment. I've never come across one where I didn't know for sure whether the link was internal or external. I guess it could happen, I'll try to keep my mind open. Regardless, making a new component to handle external links seems like a smart encapsulation of concerns, as the definition of that problem is things that are external to the router.

Handling placeholder links within the router context seems firmly within the realm of things that the library should accommodate. The W3C and WHATWG decided that placeholder links are totally valid. Whether you agree with the usefulness of this feature or not, is it wise for React to deviate from the HTML standard?

W3C https://www.w3.org/TR/html-markup/a.html#a.attrs.href
WHATWG https://html.spec.whatwg.org/#attr-hyperlink-href

The workarounds for it are simple but lame. Please consider again adding this to your library.

@taion
Copy link
Contributor

taion commented Aug 15, 2016

I'm open to reconsidering this. Thoughts @reactjs/routing?

@taion taion reopened this Aug 15, 2016
@silouanwright
Copy link

silouanwright commented Aug 31, 2016

Apologies in advance if any of my nomenclature is wrong.

I second @macgyver's suggestion. The main issue is that I know something is a link but maybe the url is generated after an action fires on the component that is supposed to mount. A javascript error is going to get thrown because that <Link> component is expecting that to parameter.

The fact of the matter is that component design is literally built around the idea of being able to pass null values until you receive the data you need from some ajax call exists.

Your only argument then is that you conditionally just create a new element or just have it not display until it's ready? That's a bad solution. After the component mounts, the user is going to just see that link pop up out of nowhere, or switch with some other element (which it shouldn't for UX purposes), unless the element is identical. But if you're going out of your way to make an element that is identical, what is the point?

If you're designing whatever substitute element you need there to look like a normal link, then it just makes sense the Link element handle that. It should have the same interface as an anchor tag.

The fact that most people haven't run into the issue just means most people are either doing the suboptimal solutions I previously ran into, or waiting until all your requests are complete before showing the page, or you're not generating any dynamic urls after the component mounts

@taion
Copy link
Contributor

taion commented Aug 31, 2016

This is an accepted feature request. If you want it, make a PR.

@sanniassin
Copy link
Contributor

sanniassin commented Jun 21, 2017

Was it intentionally reverted with #4492? Want to make a PR if it's not.

#3803 – removes isRequired flag for to
#4492 – brings it back

@maisano
Copy link
Contributor

maisano commented Jun 21, 2017

some context: it wasn't reverted, per se, in #4492, as that was happening on the v4 branch, which was a complete rewrite. that pr was opened to prevent the runtime exception that would occur within the onClick from not providing a to prop. i truly don't understand using <Link>–a react-router-specific, context-aware, intentful component–in a generic way. that said, it should be fairly simple to revert my commit and code around the absence of a to prop.

@silouanwright
Copy link

silouanwright commented Jun 28, 2017

@maisano

i truly don't understand using –a react-router-specific, context-aware, intentful component–in a generic way.

I explained above in my response. But let's try again, bullet point style.

  • Link is essentially an interface for an anchor, provided for ease of use. It's entirely possible to redirect the user yourself with what React Router provides. But the convenience makes <Link />'s use worthwhile. So <Link /> is a convenience component.

  • Tons of constructs we use within React are designed to fail silently if certain data isn't present or known.. and this is one of the great parts of React. Furthermore, it's a big assumption to make that the to prop is known at the time <Link /> is rendered, and this isn't just a React concept.

From the HTML 4.01 spec (just as true for HTML 5):

Authors may also create an A element that specifies no anchors, i.e., that doesn't specify href, name, or id. Values for these attributes may be set at a later time through scripts.

Now all that being said, it's not too hard for developers to handle this themselves.

  1. Make a wrapper component.
  2. If the to prop is passed in, render a <Link />, otherwise an <a />.
  3. Make sure all the props passed on the wrapper component get applied to whatever element ends up displaying.

But this seems weird for <Link /> to not just handle this out of the box, given that it's a component about convenience to begin with.

@JakeGinnivan
Copy link

I just wanted to chime in with our use case. We have elements which support loading skeletons, part of the reasons of having the skeletons is to minimise the amount of DOM changes when we finish loading.

Our components are wrapped in a Link. Because we can't have an empty to I have had to preventDefault, and use # as the value until we load the data. Otherwise we will unmount the dom under link because the JSX structure has changed

@silouanwright
Copy link

silouanwright commented Nov 1, 2017

@JakeGinnivan that's an exact use case for what I was trying to get across in the post I made above yours. I don't think the authors are a fan but it's been half a year and I haven't gotten a response to the points I made. Which is fine, we're all busy. But I wouldn't wait for them to implement this feature if you need this solved now.

While not ideal, I'd add a utility component called Link , wrap react-router's Link, and either import your Link everywhere that you use it, or (I've never tried this) add a webpack alias so that anytime you require their Link component, it imports your own custom Link. And this is the gist of what your custom component should contain:

let ButtonElement = props.to ? Link : 'a'

Then you return ButtonElement and spread all the props on it. Easy peasy.

@remix-run remix-run deleted a comment from MarkusJ13 Jan 31, 2018
@remix-run remix-run deleted a comment from Seerat-Ahmed May 18, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 17, 2018
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

9 participants