Skip to content

Add children prop feature in <Miss> #4140

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

Closed

Conversation

supasate
Copy link
Contributor

@supasate supasate commented Nov 4, 2016

Fix #4026

Always render a provided children prop for regardless of route matching.

@supasate
Copy link
Contributor Author

supasate commented Nov 8, 2016

@timdorr Could you help review this PR?

const { noMatchesInContext } = this.state
const { serverRouter, match } = this.context
const noMatchesOnServerContext = serverRouter &&
serverRouter.missedAtIndex(match.serverRouterIndex)
if (noMatchesInContext || noMatchesOnServerContext) {
const matched = !(noMatchesInContext || noMatchesOnServerContext)
Copy link

@maddie927 maddie927 Nov 15, 2016

Choose a reason for hiding this comment

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

Is this intentionally negated? It calls children({ matched: false }) when no other matches on context were rendered, making it behave the opposite of Match's children.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The matched property of children indicates whether it's matched or not, doesn't it? So, when no other matches on context, it should be set to false.

I think the Match's children behaves the same way. If it's matched, the matched property of its children is set to true, otherwise, false.

Please correct me if I understand something wrong.

Choose a reason for hiding this comment

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

The matched passed to Match's children only indicates if that particular Match was matched. If that wasn't the case it wouldn't be very useful. Miss is supposed to "match" when no Matches "matched", so it's inverse. Basically, if it a particular Match or Miss should render stuff then it should set matched to true.

Copy link

@maddie927 maddie927 Nov 16, 2016

Choose a reason for hiding this comment

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

The idea is that children should let you accomplish the same thing as render, but allowing the user to control what happens when it should or shouldn't render. The example in the docs is for animation, where an animation wrapper needs to always be rendered but it only has children if it's a match.

Copy link

@maddie927 maddie927 Nov 16, 2016

Choose a reason for hiding this comment

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

(see the example here under "children: func")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, when there is no match in context, it means it matches the Miss component and its children know that the parent (Miss) is matched. Got it. Thank you @spicydonuts. Will fix it then.

@supasate supasate force-pushed the add-children-prop-feature-in-Miss branch from afde7b0 to 117e259 Compare November 17, 2016 02:14
@supasate supasate force-pushed the add-children-prop-feature-in-Miss branch from 117e259 to 072e8b8 Compare December 1, 2016 04:45
@supasate
Copy link
Contributor Author

supasate commented Dec 1, 2016

Just rebased with the new Miss component

@timdorr timdorr added this to the v4.0.0 milestone Dec 5, 2016
@mjackson
Copy link
Member

The troublesome <Miss> component is gone in v4, so this PR is outdated. Thank you anyway for your work :)

@mjackson mjackson closed this Jan 11, 2017
@supasate supasate deleted the add-children-prop-feature-in-Miss branch January 11, 2017 04:18
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants