Skip to content
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

Feature for render callback of stack screen #32

Merged
merged 3 commits into from
Jun 30, 2020

Conversation

a-c-sreedhar-reddy
Copy link
Contributor

Closes #31

@MoOx
Copy link
Member

MoOx commented Jun 2, 2020

This introduce a cost for the binding and a door to bugs.
Why not keeping a simple binding with 2 props like you mentioned here #31 (comment) ? Maybe the binding should not be responsible for checking that you use the lib correctly...

@a-c-sreedhar-reddy
Copy link
Contributor Author

a-c-sreedhar-reddy commented Jun 2, 2020

But with that both children and component can be None or both of them can be provided, which would lead to app crash.

@MoOx
Copy link
Member

MoOx commented Jun 2, 2020

react navigation really trigger a crash if you forget component or children?

@a-c-sreedhar-reddy
Copy link
Contributor Author

a-c-sreedhar-reddy commented Jun 2, 2020

Both should not be passed.
WhatsApp Image 2020-06-02 at 2 17 43 PM

Both should not be undefined.
WhatsApp Image 2020-06-02 at 2 17 43 PM (1)

@MoOx
Copy link
Member

MoOx commented Jun 2, 2020

If I am correct this error happen on first launch, when the routing is being processed right? Isn't it acceptable to have this "runtime error" with zero cost bindings?

@MoOx
Copy link
Member

MoOx commented Jun 2, 2020

I am thinking about another zero cost solution:

implement 2 components Screen (component) & ScreenWithCallback (render prop)

@a-c-sreedhar-reddy
Copy link
Contributor Author

Implemented two components Screen and ScreenWithCallback.

@a-c-sreedhar-reddy a-c-sreedhar-reddy changed the title changed prop from component to componentOrRenderCallback Feature for render callback of stack screen Jun 2, 2020
"route": route,
}),
),
children: option(renderCallbackProp => React.element),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead we should have screenProps & screenPropsWithCallback?

Copy link
Contributor Author

@a-c-sreedhar-reddy a-c-sreedhar-reddy Jun 11, 2020

Choose a reason for hiding this comment

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

https://github.com/reason-react-native/reason-react-navigation/blob/master/src/Stack.re#L288

But argument of Screen function here can be only of one type right?

@Freddy03h
Copy link
Member

I'm really interested on this change!
But, maybe we should do the same change for (Bottom/Material)Tab Screen?

@MoOx
Copy link
Member

MoOx commented Jun 17, 2020

@Freddy03h Would make sense indeed. @a-c-sreedhar-reddy can you handle this?

@MoOx
Copy link
Member

MoOx commented Jun 28, 2020

@a-c-sreedhar-reddy up about the request :) Do you want us to handle it if you don't have time?

@a-c-sreedhar-reddy
Copy link
Contributor Author

Yes @MoOx .🙂

@MoOx
Copy link
Member

MoOx commented Jun 30, 2020

@Freddy03h would you like to handle that in another PR?

@MoOx MoOx merged commit a7d40f0 into rescript-react-native:master Jun 30, 2020
@Freddy03h
Copy link
Member

Ok I'll try to duplicate the changes on the tabs navigators

@MoOx
Copy link
Member

MoOx commented Jun 30, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Binding for Stack-component render-callback .
3 participants