Details
Verify the orientation is set correctly
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
native/components/full-screen-view-modal.react.js | ||
---|---|---|
1244–1256 | I wonder if a single effect would be cleaner: React.useEffect(() => { if (isActive) { Orientation.unlockAllOrientations(); } else { Orientation.lockToPortrait(); } return () => { if (isActive) { Orientation.lockToPortrait(); }; }; }, [isActive]); |
native/components/full-screen-view-modal.react.js | ||
---|---|---|
1244–1256 | Arguably it could be even more simple: React.useEffect(() => { if (!isActive) { return undefined; } Orientation.unlockAllOrientations(); return () => { Orientation.lockToPortrait(); }; }, [isActive]); |
native/components/full-screen-view-modal.react.js | ||
---|---|---|
1242 | replaced with an invariant | |
1244–1256 | We had similar discussions here and a couple of versions: https://phab.comm.dev/D14221 and we finally ended up with this version. In your version if isActive changes, the cleanup is run first then the effect and it's messy. In the version in the diff it's really clear:
| |
1254 | It doesn't really matter, if isActive is false, then we had called lockToPortrait before and here we call it again. I don't see anything wrong with calling it twice in a row. |
native/components/full-screen-view-modal.react.js | ||
---|---|---|
1244–1256 |
Not sure I agree that it's messy, but I don't feel strongly |
updateChangedUndirectedRelationships only updates the relationship if there was none, or the relationship status provided it greater than the one in the db. This will not be the case here, because KNOW_OF is 0-the lowest possible value. So we will not override a more meaningful relationship.