Page MenuHomePhabricator

[native] Moved fixed tooltip to the bottom of chat window
ClosedPublic

Authored by ginsu on Oct 28 2022, 10:42 AM.
Tags
None
Referenced Files
F3366051: D5497.diff
Mon, Nov 25, 9:19 AM
F3364245: D5497.diff
Mon, Nov 25, 3:47 AM
F3359940: D5497.id18207.diff
Sun, Nov 24, 11:43 AM
F3359938: D5497.id17966.diff
Sun, Nov 24, 11:43 AM
F3359937: D5497.id.diff
Sun, Nov 24, 11:42 AM
Unknown Object (File)
Fri, Nov 22, 3:10 PM
Unknown Object (File)
Fri, Nov 22, 10:09 AM
Unknown Object (File)
Wed, Oct 30, 9:56 AM

Details

Summary

Moved the fixed tooltip to the bottom of the chat window. Will be adding the more button/polishing animation in upcoming diffs


Depends on D5460
Linear Task: ENG-2037
Design: Figma

Test Plan

Please watch the demo video to see how all the tooltips now work with the new changes I made

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Generally looks good! Just a quick question

native/chat/multimedia-message.react.js
242 ↗(On Diff #17966)

Do we need to pass in chatContext, or can we instead use the context in MultimediaMessage? Based on my understanding of Context, it eliminates the necessity of passing values in through its props.

native/chat/multimedia-message.react.js
242 ↗(On Diff #17966)

Since chatContext holds the information regarding the current height of the input bar, we need to pass in the chatContext here. We also can't call the context inside of MultimediaMessage since it is a class based component and can't use the useContext hook

native/chat/multimedia-message.react.js
242 ↗(On Diff #17966)

So I actually found this out last week, but apparently we can get the Context within a Class based component, without needing the useContext hook (https://reactjs.org/docs/context.html#classcontexttype). It would look something like

class MultimediaMessage extends React.PureComponent<Props, State> {
     static contextType = ChatContext;

      // Now we can do something like this.context?.chatInputBarHeights....
}

Whether or not it's better to pass it in as a Prop or access it in the Class directly, probably defer to someone more experienced! For reference, I did this in this diff, https://phab.comm.dev/D5515?vs=18028&id=18032#toc, before I ended up refactoring it out since I no longer needed it.

native/chat/multimedia-message.react.js
242 ↗(On Diff #17966)

oh sweet, thanks for sharing that with me! I didn't know that was possible. I am also curious if there are any benefits/tradeoffs to doing it that way...

CC: @ashoat @atul

native/chat/multimedia-message.react.js
242 ↗(On Diff #17966)

I think it's definitely better to do as a hook, like @ginsu is doing here

Whether or not it's better to pass it in as a Prop or access it in the Class directly, probably defer to someone more experienced!

The pattern we typically follow is wrapping a ClassComponent with a functional ConnectedClassComponent that has all of the "hooks" which we pass the values of into the inner ClassComponent via props.

So yeah we'd want to use useContext hook in the wrapping ConnectedBlahComponent

atul added inline comments.
native/navigation/tooltip.react.js
336 ↗(On Diff #17966)

We could probably just use dimensions.height here directly instead of declaring fullScreenHeight on line 327

This revision is now accepted and ready to land.Nov 2 2022, 6:46 PM

addressed atul's comments

In D5497#163477, @atul wrote:

The pattern we typically follow is wrapping a ClassComponent with a functional ConnectedClassComponent that has all of the "hooks" which we pass the values of into the inner ClassComponent via props.

Ah, that makes sense - thanks for the explanation!