Details
- Reviewers
tomek inka rohan - Commits
- rCOMMf481aafdeb2f: [native] Make chat mentions clickable
Before testing, please apply the entire native stack.
- Test if mentions inside a spoiler are properly rendered and are clickable only when spoiler is opened:
- Test if mentions are clickable and if animation is changed when tooltip is opened:
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/chat/message-list-types.js | ||
|---|---|---|
| 103–110 ↗ | (On Diff #30110) | This code is a little bit similar to useAnimatedNavigateToSidebar and useAnimatedMessageTooltipButton. Before navigating to thread, currentTransitionSidebarSourceID and sidebarAnimationType is set in ChatContext only when TextMessageTooltip is visible. That's because of this issue. Why it is being set only when tooltip is visible? It's because currentTransitionSidebarSourceID is not changed to default null value. If you remove if clause, you can reproduce that error this way:
I also thought about refactoring useAnimatedMessageTooltipButton a little bit. What we could do is move this code: React.useEffect(() => {
return () => setCurrentTransitionSidebarSourceID(null);
}, [setCurrentTransitionSidebarSourceID]);into text-message.react.js. Why this bug occurs? Above useEffect runs when useAnimatedMessageTooltipButton is used =>TextMessageTooltip is mounted. |
| native/markdown/markdown-utils.js | ||
|---|---|---|
| 106–115 ↗ | (On Diff #30110) | I don't understand what the point of this function is.
What does this do that useNavigateToThreadWithFadeAnimation doesn't? |
| native/markdown/markdown-utils.js | ||
|---|---|---|
| 106–115 ↗ | (On Diff #30110) | While I was working on this task, I was playing with setLinkModalActive and realised that there is no need to use this (initially I was setting linkModalActive before and after navigateToThreadWithFadeAnimation). So I deleted setLinkModalActive lines and left this alias code... Will fix that! |
Feel free to let me know if I'm missing something here with my comments. Requesting changes so it goes back to your queue
| native/chat/message-list-types.js | ||
|---|---|---|
| 97–98 ↗ | (On Diff #30224) | Since you've already checked whether chatContext exists using the invariant above, you should be able to remove the optional chaining here. Also it might be easier if you just destructure, but up to you |
| 107–108 ↗ | (On Diff #30224) | If you remove the optional chaining above since you've used an invariant, you should just be able to do this |
Just one thing inline
| native/chat/message-list-types.js | ||
|---|---|---|
| 104–105 ↗ | (On Diff #30227) | I think you should either do what you did before after line 96 const setSidebarSourceID = chatContext.setCurrentTransitionSidebarSourceID; const setSidebarAnimationType = chatContext.setSidebarAnimationType; or destructure it after line 96 const { setCurrentTransitionSidebarSourceID: setSidebarSourceID, setSidebarAnimationType } = chatContext;(choose whatever name you want, either setCurrentTransitionSidebarSourceID or setSidebarSourceID is fine) So you can use them directly here |
Did you test whether the animation still works when user opens tooltip and presses "thread", even if the message has a link?
Then why are we keeping styles like border and border-radius?
Don't these styles also affect the <textarea> tag? Are you sure we want to be removing these styles for that as well? Or should we break this into two separate selectors (one for input and another for textarea)?