Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Fix typings of Route #217

Merged
merged 1 commit into from
Feb 13, 2019
Merged

Fix typings of Route #217

merged 1 commit into from
Feb 13, 2019

Conversation

conradreuter
Copy link
Contributor

The full router context is passed to component, render and children. This should be reflected in the types.

@CLAassistant
Copy link

CLAassistant commented Jan 14, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jan 14, 2019

Codecov Report

Merging #217 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #217   +/-   ##
=======================================
  Coverage   79.26%   79.26%           
=======================================
  Files          10       10           
  Lines         217      217           
  Branches       47       47           
=======================================
  Hits          172      172           
  Misses         30       30           
  Partials       15       15
Impacted Files Coverage Δ
src/modules/Route.js 75% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa8555a...84edf67. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jan 14, 2019

Codecov Report

Merging #217 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #217   +/-   ##
=======================================
  Coverage   79.26%   79.26%           
=======================================
  Files          10       10           
  Lines         217      217           
  Branches       47       47           
=======================================
  Hits          172      172           
  Misses         30       30           
  Partials       15       15
Impacted Files Coverage Δ
src/modules/Route.js 75% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa8555a...84edf67. Read the comment docs.

Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the PR. The only thing I wasn't sure about was the render component props. Let me ask/look around.

@@ -20,26 +20,20 @@ const isEmptyChildren = (children: React.Node) =>
type PropsType = {
trackingId?: any,
component?: React.ComponentType<*>,
render?: (...props: any) => React.Node,
children?: ((routeProps: {match: MatchType}) => React.Node) | React.Node,
render?: (routeProps: ContextRouterType) => React.Node,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to look into this, but isn't there additional props that are possible here? Maybe we need to some flow fixtures in the repository.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -20,26 +20,20 @@ const isEmptyChildren = (children: React.Node) =>
type PropsType = {
trackingId?: any,
component?: React.ComponentType<*>,
render?: (...props: any) => React.Node,
children?: ((routeProps: {match: MatchType}) => React.Node) | React.Node,
render?: (routeProps: ContextRouterType) => React.Node,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@conradreuter
Copy link
Contributor Author

Any reason why this has not been merged yet?

@KevinGrandon
Copy link
Contributor

Thanks for the reminder.

@KevinGrandon
Copy link
Contributor

!merge

@old-fusion-bot old-fusion-bot bot merged commit 17aa5bd into fusionjs:master Feb 13, 2019
@old-fusion-bot
Copy link

Triggered Fusion.js build verification: https://buildkite.com/uberopensource/fusion-release-verification/builds/1466

This was referenced Feb 19, 2019
This was referenced Feb 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants