move can promote logic to promote thread hook for reuse in web project
Details
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
lib/hooks/promote-sidebar.react.js | ||
---|---|---|
30 ↗ | (On Diff #11728) | I think this variable could be named better. What does it represent? If it's the parentThreadID in all cases, maybe we can name it that, and we could just always use it instead of ever checking threadInfo.parentThreadID |
lib/hooks/promote-sidebar.react.js | ||
---|---|---|
30 ↗ | (On Diff #11728) | agreed, that makes sense. |
I looked at this again, maybe for a bit too long and I'm having trouble figuring out exactly what you mean by @ashoat:
maybe we can name it that, and we could just always use it instead of ever checking threadInfo.parentThreadID
From what I understand about the feature, I think all we'd ever want is the threadInfo.parentID because a sidebar will always have a parentThread to be promoted from. I'm honestly not sure why this check was here in the first place.
If you have a minute today could we chat about it IRL.
lib/hooks/promote-sidebar.react.js | ||
---|---|---|
30 ↗ | (On Diff #11728) |
Putting this back as needs review to get it back into @ashoat's queue to make sure he'll see my comment.
lib/hooks/promote-sidebar.react.js | ||
---|---|---|
30 ↗ | (On Diff #11807) |
Assuming you mean threadInfo.parentThreadID... it seems like you're saying we don't need this parameter here, since we can get it from threadInfo.parentThreadID? In that case, let's simplify |
40 ↗ | (On Diff #11807) | My feedback from the last review cycle is basically that I don't think we should have two distinct places we are getting parentThreadID from. If there is some reason we can't trust threadInfo.parentThreadID, then I think this line should be handled by whoever calls usePromoteSidebar. That way, the third parameter to usePromoteSidebar will always be valid, and we never need to check threadInfo.parentThreadID. We could simplify lift this line to the caller and that would address my feedback (That said, see my comment above – if we can trust threadInfo.parentThreadID, it seems cleaner to remove the third parameter here and just use that) |
I need to test the functionality a bit before passing this over for review.
lib/hooks/promote-sidebar.react.js | ||
---|---|---|
30 ↗ | (On Diff #11807) | yes, that's the assumption! Awesome, thanks. |
Tested promote thread functionality on both iOS and web and it's working as expected.
lib/hooks/promote-sidebar.react.js | ||
---|---|---|
40 ↗ | (On Diff #12036) | This is a copy/pasta from: https://github.com/CommE2E/comm/blob/master/native/chat/settings/thread-settings.react.js#L1119 It's not that obvious from the diff because we still need it in thread-settings.react / I didn't remove it in that file. |
lib/hooks/promote-sidebar.react.js | ||
---|---|---|
36 ↗ | (On Diff #12036) | What's the reason for this added newline? |
39 ↗ | (On Diff #12036) | Can we git rid of ?? false? It's best for variables to have clean types, eg. ?string rather than ?string | boolean |
39 ↗ | (On Diff #12036) | I think it's better to use the consistent name parentThreadID that we use everywhere in the codebase |
39 ↗ | (On Diff #12036) | Can you remind me why you were passing this in as a third parameter? Are you 100% sure we can always trust threadInfo.parentThreadID? |
39 ↗ | (On Diff #12036) | In what scenario will threadInfo be falsey? It's typed as ThreadInfo, so I don't understand why you need the question mark in threadInfo?.parentThreadID |
diff review
lib/hooks/promote-sidebar.react.js | ||
---|---|---|
36 ↗ | (On Diff #12114) | going to get rid of this one too. |
39 ↗ | (On Diff #12036) | yeah, makes sense. |
39 ↗ | (On Diff #12036) | I'd say I'm about 90%. Here are the following steps I took to confirm that we can trust threadInfo.parentThreadID
|
Sending to your queue so we can discuss offline (see inline)
lib/hooks/promote-sidebar.react.js | ||
---|---|---|
38 ↗ | (On Diff #12116) | Hmm... RE trusting parentThreadID, the only reason I suspect we might have for not trusting parentThreadID is if it was a pending thread. Can you check with me in person / schedule some time on my Calendly so we can look into this together? I also want to explain the concept of a pending thread to you a bit |