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

incorrect dimensions on first layout when using forceNonDeterministicRendering #195

Closed
tafelito opened this issue Jun 9, 2018 · 10 comments

Comments

@tafelito
Copy link
Contributor

tafelito commented Jun 9, 2018

@naqvitalha I'm trying to use forceNonDeterministicRendering to show a list of cards (classic news feed) but on first render the height of the items are not being calculated. Once you scroll down and up, everything looks fine.

I'm sure this is not an issue and I'm missed something, can you point me on the right direction? you can see it here https://codesandbox.io/s/k058on6r15

If I remove the View on line 84 on App.js, it doesn't even show anything. On my local version it does show, but it shows random items per row. Trying to find out what's different. But still, it's not full width

If I show the image by itself, without RLV, the image appears full width

I'm using an external scroll viewer to make it responsive as you mentioned on #151 but using RNW. Are you planning to change the export of ScrollViewer?

I'm testing it with 1.4.0-beta.4

@naqvitalha
Copy link
Collaborator

naqvitalha commented Jun 9, 2018

Check ImageRenderer.js line 32. You've set flex:1 and height together. That won't work. Remove flex:1 and check. Check this out: https://codesandbox.io/s/pwj5pxornq

I've also removed flex:1 from two other places one being App.js: 83. This was an actual layout specification issue, you don't need flex: 1 in places where you want WRAP_CONTENT kind of behaviour.

@tafelito
Copy link
Contributor Author

tafelito commented Jun 9, 2018

Did you test it on chrome? Because I'm still seeing the same issue. I tested on Safari and it does work, but mine as well

And if you remove the View below ImageRender at line 83, nothing renders

@naqvitalha
Copy link
Collaborator

Yeah checked on chrome. Can you send me a screenshot of the layout that you are seeing and what is expected?

@tafelito
Copy link
Contributor Author

tafelito commented Jun 10, 2018

This is what I'm seeing

image

after scrolling down and up again, this is what I see

image

btw, does forceNonDeterministicRendering have any sort of caching? I didn't see any of that looking at the code. Did you think on implementing something like that?

@naqvitalha
Copy link
Collaborator

I've got the problem. Check the sample https://codesandbox.io/s/32zmm4rj91. If you're handling onSizeChanged yourself you need to override the onLayout/onSizeChanged on ScrollView to prevent RLV compute. The issue was that onSizeChanged was getting triggered twice in the same flow so before RLV could correct its layout LayoutManager itself was getting removed.
Regarding the other part non deterministic layouts are already cached by the LayoutManager.

@tafelito
Copy link
Contributor Author

OK I see, so I can actually remove the parent view of the SV and call onLayout from it. That works

But I'm still seeing some render issues. The Image itself has no width, so If I remove the views that have any text on them, then nothing is rendered. If I leave a short text, the the image takes the width of the text. https://codesandbox.io/s/6z72proovw

Another thing I noticed is that you scroll down a bit and then scroll back up, you can see the list jumping around when the rows adjust their size. In case you can't see it, here's a video

jun-11-2018 17-44-44

@naqvitalha
Copy link
Collaborator

Image size is a react native thing. They have to be inferable otherwise nothing renders unlike web where you can expect image to render and then layout accordingly. In RN you need explicit size.

Other part, things are jumping because you've randomised image height thus, with every recycle height of cell will randomly change and RLV will have to relayout.

@tafelito
Copy link
Contributor Author

make sense I completely forgot about the inferable sizes, that's actually documented somewhere.

and about the other stuff, you're also right, silly mistake.
Thanks again for the help!

btw, is there any way to remove the layout animation. I know you just added that functionality but are you planning on documenting on generating an example about the item animations and stable ids? It looks great! If not I'll have to dig into the source right away!

@naqvitalha
Copy link
Collaborator

You can check the ItemAnimator code. It is pretty well documented. In future, I might add articles around it. Right now not getting enough time.

@naqvitalha
Copy link
Collaborator

I'll close this one. Please open another incase you face issues with animators.

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

No branches or pull requests

2 participants