Page MenuHomePhabricator
Differential D4921

[web] Use new tooltip in robotext message
ClosedPublic
Actions

Authored by • jacek on Aug 23 2022, 5:11 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 8, 2:13 AM
Unknown Object (File)
Fri, Dec 6, 1:59 PM
Unknown Object (File)
Tue, Nov 26, 7:07 AM
Unknown Object (File)
Mon, Nov 25, 3:51 PM
Unknown Object (File)
Mon, Nov 25, 3:40 PM
Unknown Object (File)
Mon, Nov 25, 2:12 PM
Unknown Object (File)
Nov 22 2024, 12:42 PM
Unknown Object (File)
Nov 11 2024, 1:52 AM
View All Files
Subscribers
ashoat

Details

Reviewers
tomek
atul
• abosh
Commits
rCOMM6f5610db9d1c: [web] Use new tooltip in robotext message
Summary

Use new tooltip API in robotext message

Test Plan

Flow; Tested with the rest of diffs stack.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• jacek created this revision.Aug 23 2022, 5:11 AM
• jacek held this revision as a draft.
Herald added a subscriber: ashoat. · View Herald TranscriptAug 23 2022, 5:11 AM
Harbormaster completed remote builds in B11539: Diff 15862.Aug 23 2022, 5:21 AM
• jacek added a parent revision: D4920: [web] Use new tooltip in composed message.Aug 23 2022, 5:29 AM
• jacek added a child revision: D4922: [web] Remove timestamp tooltip from `ChatMessageList`.
• jacek published this revision for review.Aug 23 2022, 8:06 AM
tomek requested changes to this revision.Aug 26 2022, 9:28 AM

This is also affected by issues from D4920. If we take an approach with unified useMessageTooltipActions from D4918 the code would be really similar. Regardless, are there any parts that can be reused between this and D4920?

web/chat/robotext-message.react.js
47 ↗(On Diff #15862)

Readonly

This revision now requires changes to proceed.Aug 26 2022, 9:28 AM
ashoat added inline comments.Aug 26 2022, 12:01 PM
web/chat/robotext-message.react.js
3 ↗(On Diff #15862)

Why is this commented out

• jacek updated this revision to Diff 16200.Sep 1 2022, 7:14 AM

used shared logic

Harbormaster completed remote builds in B11797: Diff 16200.Sep 1 2022, 7:22 AM
tomek requested changes to this revision.Sep 2 2022, 3:36 AM
tomek added inline comments.
web/chat/robotext-message.react.js
43–44 ↗(On Diff #16200)

Is it possible for one of these to be truthy and the other falsy? I think it should never happen.

This revision now requires changes to proceed.Sep 2 2022, 3:36 AM
• jacek added inline comments.Sep 2 2022, 5:30 AM
web/chat/robotext-message.react.js
43–44 ↗(On Diff #16200)

You're right - it's due to flow error in line 49, as InlineSidebar must get non-optional ThreadInfo.

• jacek added inline comments.Sep 2 2022, 5:33 AM
web/chat/robotext-message.react.js
43–44 ↗(On Diff #16200)

However, introducing containsInlineSidebar prop is probably redundant here, so I'll leave only if(this.props.item.threadCreatedFromMessage) as before

• jacek updated this revision to Diff 16242.Sep 2 2022, 5:35 AM

remove containsInlineSidebar prop

Harbormaster completed remote builds in B11837: Diff 16242.Sep 2 2022, 5:44 AM
tomek accepted this revision.Sep 2 2022, 6:33 AM
This revision is now accepted and ready to land.Sep 2 2022, 6:33 AM
Closed by commit rCOMM6f5610db9d1c: [web] Use new tooltip in robotext message (authored by • jacek). · Explain WhySep 5 2022, 3:13 AM
This revision was automatically updated to reflect the committed changes.
• jacek added a commit: rCOMM6f5610db9d1c: [web] Use new tooltip in robotext message.

Revision Contents
Changeset List

PathSize
web/
chat/
9 lines
88 lines

Diff 16242

View Options

web/chat/message.react.js

Loading...
View Options

web/chat/robotext-message.react.js

Loading...