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

How to use react-router #1059

Closed
pmendelski opened this Issue Dec 16, 2016 · 20 comments

Comments

Projects
None yet
5 participants
@pmendelski

pmendelski commented Dec 16, 2016

Please could you specify how to use react-router with react-toolbox? It's related with #984 .

Seems that right now it's not possible without creating a lot of glue code. No way to setup the typical routing behavior for a Single Page Applications is a major drawback.

Possible solutions:

  • Create a documentation showing how to do it without breaking any feature or styling of react-toolbox
  • Create a possibility to configure behavior for all components that behave like links
  • Create react-toolbox-router that solves all router-related problems
@pmendelski

This comment has been minimized.

Show comment
Hide comment
@pmendelski

pmendelski Dec 17, 2016

There is a lot of code to be copied to integrate with react-router. See: https://github.com/borela/react-toolbox-plus-router

pmendelski commented Dec 17, 2016

There is a lot of code to be copied to integrate with react-router. See: https://github.com/borela/react-toolbox-plus-router

@pmendelski

This comment has been minimized.

Show comment
Hide comment
@pmendelski

pmendelski Dec 17, 2016

Using HOC breaks the styling.

Example:

  <List selectable ripple>
    <ListItem caption="Undecorated ListItem" to="/about" />
    <DecoratedListItem caption="Decorated ListItem" to="/about"  />
  </List>

DecoratedListItem - has no hover effect.

pmendelski commented Dec 17, 2016

Using HOC breaks the styling.

Example:

  <List selectable ripple>
    <ListItem caption="Undecorated ListItem" to="/about" />
    <DecoratedListItem caption="Decorated ListItem" to="/about"  />
  </List>

DecoratedListItem - has no hover effect.

@javivelasco

This comment has been minimized.

Show comment
Hide comment
@javivelasco

javivelasco Dec 17, 2016

Member

In that case, you are decorating a ListItem component that is a dependency of List component. This is happening because the List is not able to see the children anymore so you have two different options to solve this:

  • Create a new List component by dependency injection of the new decorated ListItem.
  • Decorate a different component such as a Link and using it as a descendant of ListItem component.
Member

javivelasco commented Dec 17, 2016

In that case, you are decorating a ListItem component that is a dependency of List component. This is happening because the List is not able to see the children anymore so you have two different options to solve this:

  • Create a new List component by dependency injection of the new decorated ListItem.
  • Decorate a different component such as a Link and using it as a descendant of ListItem component.
@javivelasco

This comment has been minimized.

Show comment
Hide comment
@javivelasco

javivelasco Dec 17, 2016

Member

I'm keeping this open to bring the example to the docs

Member

javivelasco commented Dec 17, 2016

I'm keeping this open to bring the example to the docs

@pmendelski

This comment has been minimized.

Show comment
Hide comment
@pmendelski

pmendelski Dec 18, 2016

Thanks for prompt response.

  • Creating new List component just to have proper links does not sound as a good solution.
  • Using ListItem with decorated Link again breaks the style. Only clicking the text inside the link triggers routing change. It cannot be solved by simple style rule.

See:

<List selectable ripple>
    <ListItem caption="ListItem" to="/about" />
    <ListItem>
      <ReactToolboxLink to="/about">DecortedLink</ReactToolboxLink>
    </ListItem>
  </List>

pmendelski commented Dec 18, 2016

Thanks for prompt response.

  • Creating new List component just to have proper links does not sound as a good solution.
  • Using ListItem with decorated Link again breaks the style. Only clicking the text inside the link triggers routing change. It cannot be solved by simple style rule.

See:

<List selectable ripple>
    <ListItem caption="ListItem" to="/about" />
    <ListItem>
      <ReactToolboxLink to="/about">DecortedLink</ReactToolboxLink>
    </ListItem>
  </List>
@javivelasco

This comment has been minimized.

Show comment
Hide comment
@javivelasco

javivelasco Dec 18, 2016

Member

It looks more like a styling issue in the second case you mentioned. Check the code and add a className to expand the anchor in the whole container element.

I'm curious about why the first one doesn't sound good to you. Think about ListItem as a dependency of List. If you change the child, you have to tell the parent in order to identify it and inject what's needed. Another alternative is to standardize a property to tell a parent component the possible types of children where it should inject extra properties.

Member

javivelasco commented Dec 18, 2016

It looks more like a styling issue in the second case you mentioned. Check the code and add a className to expand the anchor in the whole container element.

I'm curious about why the first one doesn't sound good to you. Think about ListItem as a dependency of List. If you change the child, you have to tell the parent in order to identify it and inject what's needed. Another alternative is to standardize a property to tell a parent component the possible types of children where it should inject extra properties.

@pmendelski

This comment has been minimized.

Show comment
Hide comment
@pmendelski

pmendelski Dec 18, 2016

Adding class or style to expand link does not solve the problem. Because anchor element is wrapped in spans that are not full width. Those spans come from ListItem. To make it full width I would need to create a custom ListItem and List.

Example:

  <List selectable ripple>
    <ListItem caption="ListItem" to="/about" />
    <ListItem>
      <ReactToolboxLink style={{ width: '100%' }} to="/about">DecortedLink</ReactToolboxLink>
    </ListItem>
  </List>

Result:

<li class="theme__listItem___30lng ">
  <span class="theme__item___3W21r theme__item___3W21r theme__selectable___sCYtG theme__selectable___sCYtG">
    <span class="theme__left___z7gaI theme__left___z7gaI theme__left___z7gaI">
      <span class="theme__itemAction___3bE-p theme__itemAction___3bE-p theme__itemAction___3bE-p theme__itemAction___3bE-p">
        <a style="width: 100%;" href="/about">DecortedLink</a>
      </span>
    </span>
    <span class="theme__itemContentRoot___2rUlN theme__itemContentRoot___2rUlN theme__itemContentRoot___2rUlN"></span>
  </span>
</li>

pmendelski commented Dec 18, 2016

Adding class or style to expand link does not solve the problem. Because anchor element is wrapped in spans that are not full width. Those spans come from ListItem. To make it full width I would need to create a custom ListItem and List.

Example:

  <List selectable ripple>
    <ListItem caption="ListItem" to="/about" />
    <ListItem>
      <ReactToolboxLink style={{ width: '100%' }} to="/about">DecortedLink</ReactToolboxLink>
    </ListItem>
  </List>

Result:

<li class="theme__listItem___30lng ">
  <span class="theme__item___3W21r theme__item___3W21r theme__selectable___sCYtG theme__selectable___sCYtG">
    <span class="theme__left___z7gaI theme__left___z7gaI theme__left___z7gaI">
      <span class="theme__itemAction___3bE-p theme__itemAction___3bE-p theme__itemAction___3bE-p theme__itemAction___3bE-p">
        <a style="width: 100%;" href="/about">DecortedLink</a>
      </span>
    </span>
    <span class="theme__itemContentRoot___2rUlN theme__itemContentRoot___2rUlN theme__itemContentRoot___2rUlN"></span>
  </span>
</li>
@pmendelski

This comment has been minimized.

Show comment
Hide comment
@pmendelski

pmendelski Dec 18, 2016

I find creating new List component, not the most elegant solution because it would break the DRY principle. It's a lot of copy-pasting for such a basic need.

Selectable property is a presentation only feature (correct?). Maybe it could be solved by a simple css rule on the List component that applies hover effect on its children?

pmendelski commented Dec 18, 2016

I find creating new List component, not the most elegant solution because it would break the DRY principle. It's a lot of copy-pasting for such a basic need.

Selectable property is a presentation only feature (correct?). Maybe it could be solved by a simple css rule on the List component that applies hover effect on its children?

@pmendelski

This comment has been minimized.

Show comment
Hide comment
@pmendelski

pmendelski Dec 18, 2016

BTW, please take into consideration that simply decorating components breaks styling not only in ListItem. Please see #984 (comment)

The main issue are themes, the only way to have it fully working "out of the box" was to clone the current code for the button, iconbutton, link and make minor changes

pmendelski commented Dec 18, 2016

BTW, please take into consideration that simply decorating components breaks styling not only in ListItem. Please see #984 (comment)

The main issue are themes, the only way to have it fully working "out of the box" was to clone the current code for the button, iconbutton, link and make minor changes

@javivelasco

This comment has been minimized.

Show comment
Hide comment
@javivelasco

javivelasco Dec 18, 2016

Member

It is not breaking DRY if you wrap for component for every List in your project. The issue is not that simple, let me explain it a little.

There are some components that set a context to its children and they manage them by injecting new properties or modifying them in some way. The case of List is simple: when a list is selectable , a selectable property is passed down. As far as I remember it is only used for styling purposes but changing it would not work in a future approach I have in mind. About this, one of the most common issues is that selectors are too specific. The best case is to have a single className per node with some modifier classnames. For this use case the classname is listItem and the modifier selectable.

The thing is that the List component needs to identify its children. Right now you can tell the List component that the ListItem is your custom in the factory function initialized internally by react-toolbox. You are free to create a custom-rt/List or even to do some magic with webpack to replace the List project wide with a custom that allows to pass item options to decorated children.

I find a better solution for this cases to add a prop in the list so you can tell it with itemComponents={[ ListItem, CustomListItem ]} so they are treated the same. What branch are you using atm?

Member

javivelasco commented Dec 18, 2016

It is not breaking DRY if you wrap for component for every List in your project. The issue is not that simple, let me explain it a little.

There are some components that set a context to its children and they manage them by injecting new properties or modifying them in some way. The case of List is simple: when a list is selectable , a selectable property is passed down. As far as I remember it is only used for styling purposes but changing it would not work in a future approach I have in mind. About this, one of the most common issues is that selectors are too specific. The best case is to have a single className per node with some modifier classnames. For this use case the classname is listItem and the modifier selectable.

The thing is that the List component needs to identify its children. Right now you can tell the List component that the ListItem is your custom in the factory function initialized internally by react-toolbox. You are free to create a custom-rt/List or even to do some magic with webpack to replace the List project wide with a custom that allows to pass item options to decorated children.

I find a better solution for this cases to add a prop in the list so you can tell it with itemComponents={[ ListItem, CustomListItem ]} so they are treated the same. What branch are you using atm?

@javivelasco

This comment has been minimized.

Show comment
Hide comment
@javivelasco

javivelasco Dec 18, 2016

Member

I'm actively working on the css-next branch. If you find it useful we can add a property to the List component to allow this along with the decorator which would be included in react-toolbox/lib/hoc/withReactRouter. What do you think?

Member

javivelasco commented Dec 18, 2016

I'm actively working on the css-next branch. If you find it useful we can add a property to the List component to allow this along with the decorator which would be included in react-toolbox/lib/hoc/withReactRouter. What do you think?

@pmendelski

This comment has been minimized.

Show comment
Hide comment
@pmendelski

pmendelski Dec 19, 2016

Hi, thanks for such a comprehensive answer.

I get your concern. selectable is just an example of a presentation property. In other composite components there are other properties that must be also passed to their children.

What is the perfect solution?
IMHO the perfect solution look like:

  <List selectable>
    <ListItem>
      <Link to="/about">Simple Link from react-router or any other library</Link>
    </ListItem>
  </List>

This way the List can operate on ListItems and react-toolbox can be used with any other routing library. The problem is that this solution breaks the styling. Maybe we could just fix the styling issue for ListItem and all the other components (Button, MenuItem, Link)?

P.S.
I'm using react-toolbox v1.3.1.

pmendelski commented Dec 19, 2016

Hi, thanks for such a comprehensive answer.

I get your concern. selectable is just an example of a presentation property. In other composite components there are other properties that must be also passed to their children.

What is the perfect solution?
IMHO the perfect solution look like:

  <List selectable>
    <ListItem>
      <Link to="/about">Simple Link from react-router or any other library</Link>
    </ListItem>
  </List>

This way the List can operate on ListItems and react-toolbox can be used with any other routing library. The problem is that this solution breaks the styling. Maybe we could just fix the styling issue for ListItem and all the other components (Button, MenuItem, Link)?

P.S.
I'm using react-toolbox v1.3.1.

@pmendelski

This comment has been minimized.

Show comment
Hide comment
@pmendelski

pmendelski Dec 19, 2016

One more thing. Please don't get me wrong. Thanks for react-toolbox :) It is the best library with material components I found.

pmendelski commented Dec 19, 2016

One more thing. Please don't get me wrong. Thanks for react-toolbox :) It is the best library with material components I found.

@pmendelski

This comment has been minimized.

Show comment
Hide comment
@pmendelski

pmendelski Dec 19, 2016

  • I wouldn't add itemComponents property. It exposes internal logic of List component.
  • I wouldn't add withReactRouter. It couples react-toolbox with react-router.

pmendelski commented Dec 19, 2016

  • I wouldn't add itemComponents property. It exposes internal logic of List component.
  • I wouldn't add withReactRouter. It couples react-toolbox with react-router.
@javivelasco

This comment has been minimized.

Show comment
Hide comment
@javivelasco

javivelasco Dec 19, 2016

Member

No problem @mendlik. The HOC actually is useful for the Button component. It is true that couples with react-router, but a lot of people is using it and it would be kept as an utility function.

About the itemComponents I'm not sure. I think there will be other cases where you want to decorate a component breaking the relationship. The perfect solution is as you describe: nesting. A lot of components are not nesting friendly and I'm willing to change it. I'm working on a roadmap to be transparent on this.

Member

javivelasco commented Dec 19, 2016

No problem @mendlik. The HOC actually is useful for the Button component. It is true that couples with react-router, but a lot of people is using it and it would be kept as an utility function.

About the itemComponents I'm not sure. I think there will be other cases where you want to decorate a component breaking the relationship. The perfect solution is as you describe: nesting. A lot of components are not nesting friendly and I'm willing to change it. I'm working on a roadmap to be transparent on this.

@javivelasco

This comment has been minimized.

Show comment
Hide comment
@javivelasco

javivelasco Jan 23, 2017

Member

This will be taken into consideration for #1030. To integrate with react router you should be able to pass the Link component as a child in almost every scenario. If this is not the case please open an issue. For the List it will be considered with the mentioned reimplementation. For cases like the Button when it is a Link itself, you can use the given hoc that will be added to the library soon.

Member

javivelasco commented Jan 23, 2017

This will be taken into consideration for #1030. To integrate with react router you should be able to pass the Link component as a child in almost every scenario. If this is not the case please open an issue. For the List it will be considered with the mentioned reimplementation. For cases like the Button when it is a Link itself, you can use the given hoc that will be added to the library soon.

@devarsh

This comment has been minimized.

Show comment
Hide comment
@devarsh

devarsh Apr 20, 2017

devarsh commented Apr 20, 2017

@devarsh

This comment has been minimized.

Show comment
Hide comment
@devarsh

devarsh Apr 20, 2017

I've also implemented a Link HOC,
https://gist.github.com/devarsh/c39a56a84dfd63e62365ffcf14f18315
Perhaps this might help

devarsh commented Apr 20, 2017

I've also implemented a Link HOC,
https://gist.github.com/devarsh/c39a56a84dfd63e62365ffcf14f18315
Perhaps this might help

@DaVinciLord

This comment has been minimized.

Show comment
Hide comment
@DaVinciLord

DaVinciLord Apr 23, 2017

@devarsh Thanks for your HOC
With react-router v4 and react-router-redux v5, I had to adapt your HOC in using "this.props.history.push(to)" at line 29.

Using this, however, react-router-redux is not firing an event but my location changes.
Do you have any idea ?

EDIT: Resolved my problem by updating my boilerplate to latest version

DaVinciLord commented Apr 23, 2017

@devarsh Thanks for your HOC
With react-router v4 and react-router-redux v5, I had to adapt your HOC in using "this.props.history.push(to)" at line 29.

Using this, however, react-router-redux is not firing an event but my location changes.
Do you have any idea ?

EDIT: Resolved my problem by updating my boilerplate to latest version

@timrsmith

This comment has been minimized.

Show comment
Hide comment
@timrsmith

timrsmith May 30, 2017

How does one use react-router link in a listItem? I would expect what @mendlik posted above to work, e.g.:

<List>
    <ListItem>
        <Link to="/somewhere">Go somewhere</Link>
    </ListItem>
</List>

but in doing so, the link appears to be ignored. Anyone have luck with this?

timrsmith commented May 30, 2017

How does one use react-router link in a listItem? I would expect what @mendlik posted above to work, e.g.:

<List>
    <ListItem>
        <Link to="/somewhere">Go somewhere</Link>
    </ListItem>
</List>

but in doing so, the link appears to be ignored. Anyone have luck with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment