Use useToggleUnreadStatus hook, remove old API call logic.
Details
- Reviewers
atul ashoat - Commits
- rCOMM6548f0c2f805: [native] [refactor] use toggle unread hook
Go to the simulator:
- Read and unread chat thread items.
- Make sure animations are still smooth.
- Go to web client and API call (that toggling read and unread) is still reflected on both native client and web client.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
native/chat/swipeable-thread.react.js | ||
---|---|---|
47 | You're directly calling close() in the render path |
The diff is really confusing, the behavior is hard to understand and reason about, and it breaks Hooks rules
native/chat/swipeable-thread.react.js | ||
---|---|---|
48 ↗ | (On Diff #10393) | This is a really confusing, unclear line. You should separate it out into two to make it more clear |
48 ↗ | (On Diff #10393) | You can't just be setting local variables like that willy-nilly from a callback. If you have some state you want to track inside a Hook, you must use useRef or useState |
48 ↗ | (On Diff #10393) | It's really weird to be setting this callback to undefined from within the callback. Even if you were doing it properly with a ref or state, it's still a confusing pattern. You should have the callback defined as a const using React.useCallback (standard), and instead have some boolean React.useRef that tracks whether a callback should be passed to useToggleUnreadStatus |
native/chat/swipeable-thread.react.js | ||
---|---|---|
48 ↗ | (On Diff #10393) | You're right. This was an oversight. When moving the current.close into a callback I forgot to remove the swipeableClose variable assignment. |
48 ↗ | (On Diff #10393) | Agreed. 200%, mutating state like this is awful, I should have caught it. |
48 ↗ | (On Diff #10393) | Agreed. This was an oversight. I don't think I need to track if the callBack was passed in or not. |
lib/hooks/toggle-unread-status.js | ||
---|---|---|
19 ↗ | (On Diff #10430) | Now that you're no longer setting the callback to undefined, there's no need to make useToggleUnreadStatus support an undefined / null callback |