faceyspacey / redux-first-router-link

<Link /> + <NavLink /> that mirror react-router's + a few additional props
MIT License
55 stars 33 forks source link

NavLink - Add a functionAsChild boolean prop to use children prop as a function #91

Open Aetherall opened 6 years ago

Aetherall commented 6 years ago

This will allow easy conditional rendering and will integrate well with many CSS-in-JS technologies ! :)

Usage:

const SuperMenu = ({classes}) => (
    <NavLink to={'/space'} functionAsChild>
        { active => (
            <FancyButton className={active ? classes.rainbow : classes.gray}>
                 { active ? "You're in space now! Whooaaa" : 'Go to space !' }
            </FancyButton>
        )}
    </NavLink>
)

If there is a super fancy way to distinguish normal childs and functionsAsChilds, it would be better to use that instead of having a functionAsChild prop

But for the time being I hope this will help somebody !

faceyspacey commented 6 years ago

very nice, for the next major version I'll try to figure out if we can do it automatically. ..cant we just check typeof children or something similar?

Aetherall commented 6 years ago

I don't think so since it would return function.. and children instanceof React.Component don't work neither, maybe checking a react specific property on the children object but it seems kind of hacky to me

faceyspacey commented 6 years ago

it's fine, here's an example:

https://github.com/acdlite/recompose/blob/master/src/packages/recompose/isClassComponent.js

Aetherall commented 6 years ago

and what if the children is not a class component but a function component ?

const FunctionComponent = () => <div>Hello !<div>

const Menu = () => (
    <nav>
        <Navlink to={'/somewhere'}><FunctionComponent/></Navlink>
    </nav>
)

Does the function still works here ?

faceyspacey commented 6 years ago

Log it and see what else we can check for. It’s fine if it’s not in the public api

Aetherall commented 6 years ago

Seems like there is plenty of possibilities !

image

We can do a

{ children.reactRelatedThing === undefined ? children(active) : children }

Ooh seems like I was wrong ! typeof does return 'object' when the children is a react component !

Aetherall commented 6 years ago

@faceyspacey Do you have an Idea on how to reduce the cognitive complexity of the PR ?

faceyspacey commented 6 years ago

First add a 3rd check for an array of components.

Then, the best way probably is to check the children.$$typeof Symbol. My first instinct was to check that _owner is defined, but if the Symbol is one we can figure out, which it of course is, and if it's very descriptive, that's probably the most "professional" way to do this.

Now that said, there likely is other symbol types, and the best thing we can do is look at how React handles these checks, which they inevitably do:

https://github.com/facebook/react/blob/b77b12311f0c66aad9b50f805c53dcc05d2ea75c/packages/react/src/ReactChildren.js#L118-L127

https://github.com/facebook/react/blob/f57d963ccea845e941f07e74c55920cd852dfc7b/packages/shared/ReactSymbols.js

Aetherall commented 6 years ago

Or maybe we could use the React dedicated function to check if it is a react component ?

image

But why not simply use typeof children === 'function' ? The only truthy case to me is when the children is a function ?

faceyspacey commented 6 years ago

I dunno. I'm not in it like you are.

Aetherall commented 6 years ago

From what I tried, The typeof implementation works, so now I need to decrease the code complexity Can you help me with this ?

faceyspacey commented 6 years ago

I don't have any time currently. I'd get it working for your purposes, fork it, post any info you can, and I'll implement this in the next major version.

Aetherall commented 6 years ago

Great thanks !!

sibnerian commented 6 years ago

Thanks for making this PR @Aetherall, super helpful. I just hit this in my own project and made a similar branch, except I check the count of the children as well.

I'm going to make a PR just to see what the "cognitive complexity" is. ✌️

ScriptedAlchemy commented 5 years ago

@sibnerian can you recheck this PR against the latest RFR2 and the latest released version of this package. If all is good I’ll merge and deploy

ScriptedAlchemy commented 5 years ago

@sibnerian bump

sibnerian commented 5 years ago

@ScriptedAlchemy sorry for the delay. I updated my PR, see #112 .