LinkListItem does not pass on "to"

[ewedlund]

Passing a "to" prop to a LinkListItem in 5.0.0 does not work, it is being consumed and not passed on to the Tag component.

https://github.com/italia/design-react-kit/blob/2acd92f76fceae1f877d93099cabfb97fed92b98/src/LinkList/LinkListItem.tsx#L47

[astagi]

Thank you @ewedlund we'll investigate and solve the problem asap.

[Nick87]

What's the difference between to and href in this component? And how are they supposed to work together?

[ewedlund]

What's the difference between to and href in this component? And how are they supposed to work together?

to is used with a react router Link, in order to do client side routing. If you use href instead, you would reload the page. You would not use them together. Something like this (at least this is what I have understood):

import { LinkList, LinkListItem } from "design-react-kit";
import { Link } from "react-router-dom";

<LinkList>
    <LinkListItem tag={Link} to="/home">
        <span>Home</span>
    </LinkListItem>
</LinkList>

I think this component should just ignore the to so that it is passed on to the Tag element through attributes, but right now it is being assigned to a variable, instead of leaving it in attributes.

[Nick87]

So we are assuming that, if the user passes the to prop, then the LinkListItem tag is a react-router-dom Link element that accepts a to prop? React Router is not the only routing library that a user can use, this way, are not we coupling a general element (the prop tag) to a specific attribute (to) that we are assuming the element accepts?

[ewedlund]

So we are assuming that, if the user passes the to prop, then the LinkListItem tag is a react-router-dom Link element that accepts a to prop? React Router is not the only routing library that a user can use, this way, are not we coupling a general element (the prop tag) to a specific attribute (to) that we are assuming the element accepts?

I think the to prop should not be declared at all in the LinkListItem component, as it would be an attribute that is transparently passed on to the component in tag. If you use a routing library that uses the prop myrouteprop, that prop would be passed on in the same way (that would work even now).

The problem is that currently to is neither being passed on, nor is it being handled in any way. But maybe I am missing something?

[Nick87]

I don't think that the tag can accept any React Element, it's an ElementType, so it can be something like h1, p, not a react router Link element nor any other element. I think that, if we want to allow managed client side redirect without a full page refresh, the whole LinkListItem must be refactored, it is not just a matter of passing a prop.

And by the way, you cannot pass a react-router-dom Link element to the prop tag:

[ewedlund]

I don't think that the tag can accept any React Element, it's an ElementType, so it can be something like h1, p, not a react router Link element nor any other element. I think that, if we want to allow managed client side redirect without a full page refresh, the whole LinkListItem must be refactored, it is not just a matter of passing a prop.

AFAIK you can pass a component to an ElementType, I have tested and it does work.

And by the way, you cannot pass a react-router-dom Link element to the prop tag:

You are passing an instance of the component, try doing

<LinkListItem tag={Link} to='myRoute'>

[Nick87]

You are correct, it does work this way, but in this case the to prop is no longer transparent as you said it should be (and I agree), and you are assuming that Link accepts a to prop, but that might not be necessarily the case as I am not assuming that Link is a react-router-dom type of element.

[ewedlund]

You are correct, it does work this way, but in this case the to prop is no longer transparent as you said it should be (and I agree), and you are assuming that Link accepts a to prop, but that might not be necessarily the case as I am not assuming that Link is a react-router-dom type of element.

Well, I do know the type of Link since I am the one passing it to the tag prop. I understand what you are saying about the to prop though, that there might be another routing framework that uses another prop. React router is however very widespread, so it would be a very common case, and from the documentation I would suspect that it was the reason why the to prop was being passed in the first place.

An alternative would be to permit passing generic extra props that would be passed to the tag (I suppose that is what the variable attributes is for?).

[astagi]

Thanks @ewedlund would you mind send a PR to solve this problem? We’ll merge it in the next version along other fixes.

[astagi]

Thanks @ewedlund .. @Nick87 which kind of refactoring are you thinking about?

For now we can remove 'to' property to avoid confusion.. it would be great to integrate an example with Link in documentation, I don't know if @ewedlund want/has time to add it.

[Nick87]

I made a few tests, and passing the to prop down to the Tag element like this:

does not do the job, since the generated html is:

We get the to attribute with the anchor tag, which does not work. For comparison, a classic <NavLink to="/login">LOGIN</Login> generates this html:

I guess that to is completely useless here and we can have the current href prop from LinkListItem work with react-router-dom as well.

This code:

with this route configuration:

redirects the user to the Login page, as expected.

[astagi]

Thank you for testing it out @Nick87

Do you agree with @ewedlund removing 'to' property as suggested by the PR and using only href with a proper routing configuration?

[ewedlund]

I made a few tests, and passing the to prop down to the Tag element like this:

So it seems that in this example you are using <LinkListItem /> without passing tag = {Link}, so it defaults to tag = 'a', which is a regular html element that does not recognize the to prop. In this case you simply should not use the to prop and just pass href.

I guess that to is completely useless here and we can have the current href prop from LinkListItem work with react-router-dom as well.

No, it is not useless: a Link does not have the href prop, and using a regular anchor will result in the page being reloaded instead of using client side routing. The fact that you see an anchor in the resulting html does not mean that it is the same thing.

[ewedlund]

Thank you for testing it out @Nick87

Do you agree with @ewedlund removing 'to' property as suggested by the PR and using only href with a proper routing configuration?

Actually in the PR I only suggested to remove the assignment of the variable to in order to pass the to prop on to the Tag element via attributes., not to remove the prop. This is surely not the best approach because it will not cover all cases, but at least it allows for the simplest routing cases.

I agree with @Nick87 that it would need some more work to make a more complete solution, either by allowing to pass any extra props that are passed on via attributes, or passing an element. In any way it would need some work to analyze the various use cases.

[Nick87]

Thank you for testing it out @Nick87

Do you agree with @ewedlund removing 'to' property as suggested by the PR and using only href with a proper routing configuration?

That's not what @ewedlund has suggested, and after @ewedlund 's comments, I need to take a closer look to check that what I thought is correct.

[astagi]

Sorry @Nick87 @ewedlund I'm on holidays atm I can only interact with my phone, not the best to look at the code :) anyway I understood the problem and the solution using 'to' along attributes.. if this problem is blocking I can accept @ewedlund solution and release a new patch version, then we can reopen the issue and continue the discussion (and possible refactoring/enhancements).. let me know

[Nick87]

Sorry @Nick87 @ewedlund I'm on holidays atm I can only interact with my phone, not the best to look at the code :) anyway I understood the problem and the solution using 'to' along attributes.. if this problem is blocking I can accept @ewedlund solution and release a new patch version, then we can reopen the issue and continue the discussion (and possible refactoring/enhancements).. let me know

I don't think this is a blocker, the to prop is just there for no reason and completely useless at the moment, so @ewedlund's PR, which removes it, is definitely good :)

[Nick87]

@ewedlund is definitely right and what I suggested earlier does not work since it causes a full page refresh, but I may be on to something that seems to be working.

With the to prop out of the game, we could use a prop tagProps, defined as:

tagProps?: Record<string, any>

containing all the props that we want to pass to the Tag component, which defaults to an empty object.

This way we can use any object as tag and pass it all the props we want, like this:

and the code above redirects the user to the correct route without triggering a full page refresh

[ewedlund]

Sorry @Nick87 @ewedlund I'm on holidays atm I can only interact with my phone, not the best to look at the code :) anyway I understood the problem and the solution using 'to' along attributes.. if this problem is blocking I can accept @ewedlund solution and release a new patch version, then we can reopen the issue and continue the discussion (and possible refactoring/enhancements).. let me know

I don't think this is a blocker, the to prop is just there for no reason and completely useless at the moment, so @ewedlund's PR, which removes it, is definitely good :)

No, my PR does not remove it, it just deletes the line that blocked it from being passed on to the tag element. It is a quick fix, so if you opt for another solution it's OK with me. It is not blocking, just annoying having to make a workaround in my code.

[ewedlund]

@ewedlund is definitely right and what I suggested earlier does not work since it causes a full page refresh, but I may be on to something that seems to be working.

With the to prop out of the game, we could use a prop tagProps, defined as:

tagProps?: Record<string, any>

containing all the props that we want to pass to the Tag component, which which defaults to an empty object.

This way we can use any object as tag and pass it all the props we want, like this:

and the code above redirects the user to the correct route without triggering a full page refresh

This was one of my proposals and fine with me. I am not sure what your approach is on other components, but in my opinion it might be a good idea to make sure that they are similar (if there are other cases like this).

E.g. in the Select component, it is only possibile to pass onChange and disabled, but I need to be able to pass required and value (and multiple but that does not seem to work in terms of visualization). It is not exactly the same case, but maybe using the same approach and name for the props to pass on could be useful?

[Nick87]

I don't think we can use with LinkListItem the same approach as Select, since here we are trying to add a prop that does not correspond to a standard html attribute. For this reason I guess the only approach is to explicitly add it.

I have taken a look to the Select element, and I found some problems that maybe I'll describe in a dedicated issue.

[Nick87]

but I need to be able to pass required and value (and multiple but that does not seem to work in terms of visualization)

@ewedlund take a look at #1042, maybe that fixes your problem

[astagi]

Good catch @Nick87! @ewedlund is @Nick87 approach right for you?

[ewedlund]

Good catch @Nick87! @ewedlund is @Nick87 approach right for you?

Not sure which one you mean but yes I think we have sorted things out, thank you to both of you!

I will close my PR since @Nick87 is taking care of it.

[Nick87]

@astagi @ewedlund do you mean that the tagProps approach is fine for you and that I should open a PR with that?

[ewedlund]

@astagi @ewedlund do you mean that the tagProps approach is fine for you and that I should open a PR with that?

So I was checking how it is handled in other components where a tag is being passed, and they do not seem to use the approach suggested by you. I am non sufficiently fluent in typescript to make suggestions but I would take a look at those other components before inventing something different.

E.g. it seems that AccordionBody permits passing any extra props directly and passes them on to the tag component: https://github.com/italia/design-react-kit/blob/61edd6eab7922a0a59566ce9d73030d933a1f99f/src/Accordion/AccordionBody.tsx#L87

(BTW I just realized that BottomNavItem needs fixing to!)

[Nick87]

You're right, my approach is not used elsewhere, and I think that your pr, #1039, is the right way of solving the issue: by removing the to prop there, it is included in attributes hence making sure it is passed on to the tag component. Sorry that I am figuring out only now what you were trying to accomplish.

[Nick87]

(BTW I just realized that BottomNavItem needs fixing to!)

what problem do you see there?

[ewedlund]

You're right, my approach is not used elsewhere, and I think that your pr, #1039, is the right way of solving the issue: by removing the to prop there, it is included in attributes hence making sure it is passed on to the tag component. Sorry that I am figuring out only now what you were trying to accomplish.

Ok, good. I still think my solution is not optimal, since it is not generic, but maybe good enough?