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

Allow View containers to specify custom measure functions #5148

Closed
wants to merge 1 commit into from

Conversation

obipawan
Copy link

@obipawan obipawan commented Jan 6, 2016

This PR allows ViewManagers that are containers (extending ViewGroupManager) to specify it's custom CSSNode.MeasureFunction.
ViewManagers that don't need to specify this can extend SimpleViewGroupManager instead

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @mkonicek, @kmagiera and @dmmiller to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 6, 2016
@mkonicek
Copy link
Contributor

mkonicek commented Jan 6, 2016

Hey @obipawan, what's the motivation for this?

@obipawan
Copy link
Author

obipawan commented Jan 6, 2016

Hey @mkonicek
This change is basically to allow custom Views that are also containers(ViewGroup) to other views to be able to specify a measure function if they wanted to. With the current ViewGroupManager, it's not possible to specify a custom LayoutShadowNode.
It also brings this in parity with iOS where one can create any native view from RCTViewManager and then specify a custom RCTShadowView.

@mkonicek
Copy link
Contributor

mkonicek commented Jan 6, 2016

OK thanks for the explanation! @kmagiera, @foghina are you OK with this?

@astreet
Copy link
Contributor

astreet commented Jan 6, 2016

Why can you not provide a custom LayoutShadowNode by extending ViewGroupManager?

@kmagiera
Copy link
Contributor

kmagiera commented Jan 6, 2016

I don't see this change being necessary to support the use case you're describing. If you want your view group manager to use custom class as a shadow node you can simply override createShadowNodeInstance and getShadowNodeClass (note that both methods are not marked as final in ViewGroupManager). TBH I'd rather avoid adding yet another base class to the namespace

@obipawan
Copy link
Author

obipawan commented Jan 6, 2016

Hey @kmagiera It's possible to override createShadowNodeInstance, but as for getShadowNodeClass you'd have to make the return type to an unchecked Class type for it to work. Though this is yet another base class, it would make it safer to have the return type checked IMO.

@kmagiera
Copy link
Contributor

kmagiera commented Jan 6, 2016

@obipawan Are you sure that is the case here? The return type on getShadowNodeClass in ViewGroupManager is Class<? extends LayoutShadowNode> (not Class<LayoutShadowNode>) which should allow you to return class object of any class that extends LayoutShadowNode.

@obipawan
Copy link
Author

obipawan commented Jan 7, 2016

@kmagiera Apologies! you're absolutely right. I missed that this was changed for 0.18 and missed this even while submitting this PR. My bad!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants