Fluentui: React Router support discussion

[fantua]

I want to discuss issue with React Router support. In CommandBar/ContextualMenu/etc. there is href prop, but it's doesn't work with React Router. Now I'm handling onClick event and then call router push method. But it's not a good solution, because it's not a link, I can't open this in new tab/copy link/etc. in browser.

What is the best way to implement this?

My vision is:

onHrefRender?: (item: React.ReactNode, anchorProps: any) => React.ReactNode;

and using:

import { Link } from 'react-router';
//...
{
    key: 'homepage',
    name: 'Home',
    onHrefRender: (item, aProps) => <Link { ...aProps } to="/index" >{ item }</Link>
};

[dzearing]

I don't totally understand why they made the Link component for react-router, which generates DOM, rather than a utility to return an href or onClick handler that they can interpret. Is there a method in react-router to generate an href for a normal link? I can't find one in their documentation.

e.g. this would make sense to me.

import { routeHref, routeClick } from 'react-router';

<IconButton href={ routeHref('/index') } />
<NavItem onClick={ routeClick('/index') } />

In this case the router can work as expected, but you can use whatever presentation you'd like.

We have logic in the BaseButton to use an anchor if there is an href, otherwise a button. That way anchors have the consistent anchor behaviors, like ctrl/cmd-clicking and status bar on hover, while non-href based things don't. But you can have them look consistent.

This is what we want to standardize on for all clicky components, so the Link, *Button, ContextualMenuItem, NavItem, etc are just variants of the base button. React-router fights against this because it wants to own the DOM element too, which adds confusion regarding the right way to handle this.

One option is to have a mode for all buttons that is "presentation only" which renders the presentation of the button without the A or the BUTTON wrapper... Then you can wrap that in a Link:

<Link to={...}><IconButton presentationOnly /></Link>

But I'm not sure about the side-effects. For example, when we add split button support, which is a DIV which wraps 2 buttons, this doesn't work.

[dzearing]

For container components like ContextualMenu and Nav, no matter what, the child render methods should always be override-able via onRender* IRenderFunction overrides, allowing you to support something like this.

[fantua]

Is there a method in react-router to generate an href for a normal link?
There is createHref, but it's do this:

console.log(this.context.router.createHref('/home'));  // /home

Generate only path, and doesn't handle click on a or something like this.

I need this only for ContextualMenu/CommandBar, Buttons I always cover via Link.
So we can add onHrefRender for ContextualMenu or add rendered Item (React.Node) to onRender via param, to allow me cover Item via Link.

[fantua]

@dzearing I think afterRender* for ContextualMenu will resolve this.

afterRender?: (renderedItem: React.ReactNode, item: any) => React.ReactNode;

If this ok, I'll create PR.

[cschleiden]

@fantua @dzearing I think we should change the ContextualMenuItem onRender prop to follow the the IRenderFunction convention of passing in the defaultRender function.

[fantua]

@cschleiden looks like that is exactly what I need!

[dzearing]

@fantua @cschleiden I'd love all onRender* methods to follow the IRenderFunction convention and would gladly welcome that change.

That said; I don't think it will improve the Link usage. Link will still want to render the A tag, so rendering a nested BUTTON or A inside of an A doesn't work correctly in the browser.

But, I could be wrong. So If you'd like to convert it to onRenderItem: IRenderFunction<IButtonProps> I think that would be awesome!

[fantua]

I can't convert ContextualMenuItem onRender prop to IRenderFunction. Because IRenderFunction accept only 1 param which always is item, but ContextualMenu render methods need more params.

private _renderNormalItem(item: IContextualMenuItem, index: number, hasCheckmarks: boolean, hasIcons: boolean): React.ReactNode {
    if (item.onRender) {
      return [item.onRender(item)];
    }
    if (item.href) {
      return this._renderAnchorMenuItem(item, index, hasCheckmarks, hasIcons);
    }
    return this._renderButtonItem(item, index, hasCheckmarks, hasIcons);
  }

How override this to IRenderFunction?

[fantua]

@dzearing @cschleiden how to solve this?

[vertonghenb]

Any thoughts on this issue? It's quite something fundamental. I'd love to pitch in if we know the direction were heading with this.

[cschleiden]

@fantua You could have something like IContextualMenuItemRenderProps that groups all of these props

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to Fabric React!

[blrchen]

So what is the suggested way to use fabric react nav and react-router together?

[stale[bot]]

This issue has been automatically marked as stale because it has not activity for 30 days. It will be closed if no further activity occurs within 14 days of this comment. Thank you for your contributions to Fabric React!
Why am I receiving this notification?

[davidhenley]

Bumping so this doesn't get closed.

[micahgodbolt]

I know this is a long time coming (especially for such a small change). But this codepen using React Router will work once the new release is pushed out tonight.

There are a few styling differences, but nothing too horrible. I'll try to update once the code is released.

https://codepen.io/micahgodbolt/pen/EOVXGQ?editors=1010

[msft-github-bot]

:tada:This issue was addressed in #7006, which has now been successfully released as [email protected].:tada:

Handy Links:

• Release Notes
• Official NPM Release
• Test the release with Developer Demo Site