Details
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. |
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
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 |