search

faceyspacey / redux-first-router-link

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

Component prop to Link and NavLink #114

Open klis87 opened 5 years ago

klis87 commented 5 years ago

I need to pass custom component to Link and Navlink to make it work with react-bootstrap components, and I made a PR for this #42 which was rejected because RUDY was supposed to be released, but it is not and won't be any time soon, would you accept this PR if I rebased to current master?

ScriptedAlchemy commented 5 years ago

It’s delayed yes. But in development.

I’ll have to consult with James and my other engineers on accepting a “for now” PR.

It will require documentation and tests. If flow is in there, please make sure flow passes.

klis87 commented 5 years ago

@ScriptedAlchemy I am aware, checking it from time to time, I cannot wait for it, will be so awesome!

But in the meantime things like component and render props would be cool to add here. I will make sure to add necessary tests and that flow is not broken, but one question though, I noticed a PRs related to render prop. Will it be merged soon? I think we should synchronize this, because I will for sure introduce conflicts by adding component, so it would be cool to add it first.

ScriptedAlchemy commented 5 years ago

Tag me in a PR and it will be so.

Docs, flow, and tests please

We’ve had a few merges that’s broke flow and caused everyone headaches :)

klis87 commented 5 years ago

@ScriptedAlchemy I just wanted to start working on it, but yarn run flow throws maaany errors, like:

  5: import type { ComponentType } from 'react'
                   ^^^^^^^^^^^^^ Named import from module `react`. This module has no named export called `ComponentType`.

so what should I do? I could add typings for new component prop, but this won't fix outstanding issues.

klis87 commented 5 years ago

@ScriptedAlchemy linting is also broken, like:

Cannot read property 'type' of undefined
TypeError: Cannot read property 'type' of undefined
    at isForInRef (/home/klis87/projects/redux-first-router-link/node_modules/eslint/lib/rules/no-unused-vars.js:406:24)
ScriptedAlchemy commented 5 years ago

Booo. Okay I’ll have to find time to look into patching this. If you happen to fork or fix anything as well PR me. Otherwise I’ll try to fix it up this week

klis87 commented 5 years ago

Cool, thx! Just wondering how it is possible that travis doesn't complain about linting and flow in recent commits :o