search

jeremybarbet / react-native-portalize

The simplest way to render anything on top of the rest.
MIT License
335 stars 22 forks source link

fix: use a unique key to track components #8

Closed benjaminreid closed 4 years ago

benjaminreid commented 4 years ago

This seems to fix the issue, the nextKey++ would always seem to generate duplicate keys in some manor. Using Date.now() produces much more random and unique keys that stops the refs from being lost.

Before

Screenshot 2020-08-10 at 11 22 48

After

Screenshot 2020-08-10 at 12 26 00

Fixes #7 Fixes #5

jeremybarbet commented 4 years ago

Hey, that looks good to me, very unlikely Date.now produce the same timestamp twice 🤔

Do you know if it also fixes this issue #5 since it's also an issue of null ?

benjaminreid commented 4 years ago

@jeremybarbet Yep, that is pretty much the exact issue. Navigating to a screen which also uses a <Portal> would make the other screen loose it’s ref 👍

jeremybarbet commented 4 years ago

Great, thank you then!

harveyappleton commented 4 years ago

Hey @jeremybarbet, thanks so much for this epic component and react-native-modalize, loving them! And thanks @benjaminreid for investigating that fix.

I'm using Portalize (with modalize) in a react native app with react navigation.

Came across an issue today: the fix @benjaminreid provided would work across multiple react navigation screens - but if I had multiple portals on one screen, Date.now() isn't sufficiently random enough - if the render process is quick enough, I had 3 portals getting the same ref on one page which meant portals would work randomly at best.

A bit new to open source contribution, but think I've come up with a fix, which i could suggest as a pull request? Go easy on me! It's just simply extending Date.now to also include a random number, which appears to work in my basic tests.

I'll give a pull request a go 👍 Thanks team!

edc123 commented 4 years ago

EDIT EDIT: lol i'm one step behind everyone and just spotted the PR being raised currently, uuid would be great

EDIT: oh shoot sorry i just read the comment above... it's exactly what i'm saying below. Happy to PR and test if no one's got the time..

Hmmm. What if there's two Modals (note: I have them wrapped in a "Drawer" component on the left)? I see the times are...exactly the same?! The first one is meant to subsequently open the second one on pressing some of the options - but both refs come back as null. Do you think this is related? Or is it an issue on how I'm using Portalize / Modalize?

Could it be Date.now() and some random number / + nextKey? haha... ha..

Screen Shot 2020-08-15 at 11 07 42 am

LucidNinja commented 4 years ago

Where's this issue at? We are having the same issue when building to production with multiple modals on one screen. Definitely need a fix for the Date issue - otherwise can open a PR - unless someone else is already onto this? Does a random number added to date work?

jeremybarbet commented 4 years ago

@LucidNinja Fix in progess here https://github.com/jeremybarbet/react-native-portalize/pull/9