Implement {,un}hideInstance on RN renderer #14115
Conversation
|
Hm. We shouldn't assume that the flattening happens the way it does today. There is a configuration layer that determines that. Perhaps we can call I was hoping that we'd be able to batch these into the normal properties update call so there is only one updateView call. I suspect this can lead to subtle bugs depending on the order of these updateView calls not getting flipped and it'll cause two updates where one is expected. However, eventually this will all go away with Fabric. We should add a fix to Fabric too at the same time so they line up. That one might be more straightforward. |
Details of bundled changes.Comparing: 8eca0ef...bb0a833 react-native-renderer
scheduler
Generated by 🚫 dangerJS |
|
You mean it's OK to hardcode {style: {display}} but not {display}? I can change that. |
|
Yes. I'm uneasy with double updateView calls. But I think I'm ok with it, considering it's legacy in a Fabric world. So the only action items are 1) avoid the hard coding 2) fix fabric. |
| instance._nativeTag, | ||
| viewConfig.uiViewClassName, | ||
| {display: 'none'}, | ||
| ); | ||
| } | ||
|
|
||
| export function hideTextInstance(textInstance: TextInstance): void { | ||
| throw new Error('Not yet implemented.'); |
There was a problem hiding this comment.
Can this happen? E.g. returning a string from render?
There was a problem hiding this comment.
if you do
<Text><Suspense>{string}</Suspense></Text>
I think so. it seems weird, I don't know if this is complicated, and we never do this right now so I opted to skip it
There was a problem hiding this comment.
I believe these are just like any other shadow node inside text from the native's perspective so I think we could send the display: none property here too.
cc @shergin
There was a problem hiding this comment.
I believe currently the only most outer <Text> supports display: none, but I think that's incorrect behavior (and I will fix it).
There was a problem hiding this comment.
I believe in the DOM version we actually just set the string to empty string "" while hidden and then reset it to its current value.
Seems like we could do the same for RN without needing to fix the native side.
sophiebits
force-pushed
the
rn-hide-unhide
branch
2 times, most recently
from
10018bf
to
48a084b
Compare
|
This OK? I don't love the diff call in unhide but I can't think of a correct way to do it better. |
| throw new Error('Not yet implemented.'); | ||
| const node = instance.node; | ||
| const updatePayload = ReactNativeAttributePayload.diff( | ||
| {...props, style: [props.style, {display: 'none'}]}, |
There was a problem hiding this comment.
What does spread compile to?
There was a problem hiding this comment.
Object.assign({}, props, { style: [props.style, { display: "none" }] }),
which is polyfilled https://github.com/facebook/react-native/blob/cc90c20bf60c72fc1b42f3306636847ce7f478e5/Libraries/polyfills/Object.es6.js#L15
sophiebits
force-pushed
the
rn-hide-unhide
branch
from
48a084b
to
53916c7
Compare
This is required to use lazy. Test Plan: * Verified lazy works on a real world use case (shows spinner, shows real content). * Verified that if I change the primary content's styles to have `display: 'none'` then it never appears (i.e., the code in `unhide` reads the styles successfully)
sophiebits
force-pushed
the
rn-hide-unhide
branch
from
53916c7
to
bb0a833
Compare
This is required to use lazy. Test Plan: * Verified lazy works on a real world use case (shows spinner, shows real content). * Verified that if I change the primary content's styles to have `display: 'none'` then it never appears (i.e., the code in `unhide` reads the styles successfully)
This is required to use lazy. Test Plan: * Verified lazy works on a real world use case (shows spinner, shows real content). * Verified that if I change the primary content's styles to have `display: 'none'` then it never appears (i.e., the code in `unhide` reads the styles successfully)
This is required to use lazy. Test Plan: * Verified lazy works on a real world use case (shows spinner, shows real content). * Verified that if I change the primary content's styles to have `display: 'none'` then it never appears (i.e., the code in `unhide` reads the styles successfully)
This is required to use lazy.
Test Plan:
display: 'none'then it never appears (i.e., the code inunhidereads the styles successfully)