Changeset View
Standalone View
web/chat/message-tooltip.react.js
Show All 9 Lines | |||||
import type { ThreadInfo } from 'lib/types/thread-types.js'; | import type { ThreadInfo } from 'lib/types/thread-types.js'; | ||||
import { | import { | ||||
tooltipButtonStyle, | tooltipButtonStyle, | ||||
tooltipLabelStyle, | tooltipLabelStyle, | ||||
tooltipStyle, | tooltipStyle, | ||||
} from './chat-constants.js'; | } from './chat-constants.js'; | ||||
import css from './message-tooltip.css'; | import css from './message-tooltip.css'; | ||||
import { useSendReaction } from './reaction-message-utils.js'; | import { | ||||
useSendReaction, | |||||
getEmojiKeyboardPosition, | |||||
} from './reaction-message-utils.js'; | |||||
import { useTooltipContext } from './tooltip-provider.js'; | import { useTooltipContext } from './tooltip-provider.js'; | ||||
import { useSelector } from '../redux/redux-utils.js'; | import { useSelector } from '../redux/redux-utils.js'; | ||||
import { type MessageTooltipAction } from '../utils/tooltip-utils.js'; | import type { | ||||
MessageTooltipAction, | |||||
TooltipSize, | |||||
TooltipPositionStyle, | |||||
} from '../utils/tooltip-utils.js'; | |||||
type MessageTooltipProps = { | type MessageTooltipProps = { | ||||
+actions: $ReadOnlyArray<MessageTooltipAction>, | +actions: $ReadOnlyArray<MessageTooltipAction>, | ||||
+messageTimestamp: string, | +messageTimestamp: string, | ||||
+alignment?: 'left' | 'center' | 'right', | +tooltipPositionStyle: TooltipPositionStyle, | ||||
+tooltipSize: TooltipSize, | |||||
+item: ChatMessageInfoItem, | +item: ChatMessageInfoItem, | ||||
+threadInfo: ThreadInfo, | +threadInfo: ThreadInfo, | ||||
}; | }; | ||||
function MessageTooltip(props: MessageTooltipProps): React.Node { | function MessageTooltip(props: MessageTooltipProps): React.Node { | ||||
const { | const { | ||||
actions, | actions, | ||||
messageTimestamp, | messageTimestamp, | ||||
alignment = 'left', | tooltipPositionStyle, | ||||
tooltipSize, | |||||
item, | item, | ||||
threadInfo, | threadInfo, | ||||
} = props; | } = props; | ||||
const { messageInfo, reactions } = item; | const { messageInfo, reactions } = item; | ||||
const { alignment = 'left' } = tooltipPositionStyle; | |||||
const [activeTooltipLabel, setActiveTooltipLabel] = React.useState<?string>(); | const [activeTooltipLabel, setActiveTooltipLabel] = React.useState<?string>(); | ||||
const { renderEmojiKeyboard } = useTooltipContext(); | const { renderEmojiKeyboard } = useTooltipContext(); | ||||
ashoat: Nit: `renderEmojiKeyboard` should probably be named `shouldRenderEmojiKeyboard`. There's a… | |||||
const [emojiKeyboardNode, setEmojiKeyboardNode] = React.useState(null); | |||||
const emojiKeyboardRef = React.useRef(); | |||||
React.useEffect(() => { | |||||
if (emojiKeyboardRef.current) { | |||||
setEmojiKeyboardNode(emojiKeyboardRef.current); | |||||
} | |||||
}, [renderEmojiKeyboard]); | |||||
tomekUnsubmitted Not Done Inline ActionsIt doesn't seem like this is going to work properly. First of all, why do we need to store the node in two places: emojiKeyboardRef and emojiKeyboardNode? Keyboard position computation is based on emojiKeyboardNode which is updated only in an effect that runs when renderEmojiKeyboard changes. It doesn't react to emojiKeyboardRef changes which were the reason for previous iterations on this diff. For me, it seems like we should store the node in only one place, in a state. But maybe I'm missing something - in that case please explain a little bit more behind this approach. tomek: It doesn't seem like this is going to work properly.
First of all, why do we need to store… | |||||
ginsuAuthorUnsubmitted Done Inline Actions
The reason I have stored the node in two places is because, from what I understand, when we store the node only in the state, we are setting emojiKeyboardNode when the emoji keyboard isn't fully rendered. I came to this conclusion when I noticed that the width of emojiKeyboardNode.getBoundingClientRect() was equal to zero. However, if we store the node initally in emojiKeyboardRef and update the state of the node with the value of emojiKeyboardRef.current whenever renderEmojiKeyboard changes, we can get the fully rendered node, and when we log out emojiKeyboardNode.getBoundingClientRect() we can see that the width equals 352, which is the expected value for the width. I used this article as a guide when I built this solution. Over the weekend, I will see if I can find alternate solutions where we can store the node just in the state.
It's never my intention when I submit code to waste anyone's time, but I am really sorry if this has been the case recently. Moving forward, if I run into situations where I am submitting code that is not an obviously clear solution, I will be sure to annote with comments with my rationale-- something that I can work on. This should require me think more critically of my solutions, learn better, and offer more context for my reviewer. ginsu: > First of all, why do we need to store the node in two places: emojiKeyboardRef and… | |||||
ashoatUnsubmitted Not Done Inline Actions
Try being more selective in the future... the code in this article shows that the author doesn't understand render cycle performance (completely ignoring React.useMemo and React.useCallback), which should be a strong signal that the author is more focused on "getting something that works" rather than something that works well. If the author put code like that up for review here, it would get picked apart. From that you should conclude that you can't really trust that author's perspective on state / refs either... solid chance that would get picked apart too. ashoat: > I used this [[ https://www.kindacode.com/article/react-get-the-width-height-of-a-dynamic… | |||||
const messageActionButtonsContainerClassName = classNames( | const messageActionButtonsContainerClassName = classNames( | ||||
css.messageActionContainer, | css.messageActionContainer, | ||||
css.messageActionButtons, | css.messageActionButtons, | ||||
); | ); | ||||
const messageTooltipButtonStyle = React.useMemo(() => tooltipButtonStyle, []); | const messageTooltipButtonStyle = React.useMemo(() => tooltipButtonStyle, []); | ||||
const tooltipButtons = React.useMemo(() => { | const tooltipButtons = React.useMemo(() => { | ||||
▲ Show 20 Lines • Show All 56 Lines • ▼ Show 20 Lines | const tooltipTimestamp = React.useMemo(() => { | ||||
} | } | ||||
return ( | return ( | ||||
<div className={css.messageTooltipLabel} style={messageTooltipLabelStyle}> | <div className={css.messageTooltipLabel} style={messageTooltipLabelStyle}> | ||||
{messageTimestamp} | {messageTimestamp} | ||||
</div> | </div> | ||||
); | ); | ||||
}, [messageTimestamp, messageTooltipLabelStyle]); | }, [messageTimestamp, messageTooltipLabelStyle]); | ||||
const emojiKeyboardPosition = React.useMemo( | |||||
() => | |||||
getEmojiKeyboardPosition( | |||||
emojiKeyboardNode, | |||||
tooltipPositionStyle, | |||||
tooltipSize, | |||||
), | |||||
[emojiKeyboardNode, tooltipPositionStyle, tooltipSize], | |||||
); | |||||
const emojiKeyboardPositionStyle = React.useMemo(() => { | |||||
if (!emojiKeyboardPosition) { | |||||
return null; | |||||
} | |||||
return { | |||||
bottom: emojiKeyboardPosition.bottom, | |||||
left: emojiKeyboardPosition.left, | |||||
}; | |||||
}, [emojiKeyboardPosition]); | |||||
const nextLocalID = useSelector(state => state.nextLocalID); | const nextLocalID = useSelector(state => state.nextLocalID); | ||||
const localID = `${localIDPrefix}${nextLocalID}`; | const localID = `${localIDPrefix}${nextLocalID}`; | ||||
const sendReaction = useSendReaction(messageInfo.id, localID, threadInfo.id); | const sendReaction = useSendReaction(messageInfo.id, localID, threadInfo.id); | ||||
const onEmojiSelect = React.useCallback( | const onEmojiSelect = React.useCallback( | ||||
emoji => { | emoji => { | ||||
const reactionInput = emoji.native; | const reactionInput = emoji.native; | ||||
const viewerReacted = !!reactions.get(reactionInput)?.viewerReacted; | const viewerReacted = !!reactions.get(reactionInput)?.viewerReacted; | ||||
const action = viewerReacted ? 'remove_reaction' : 'add_reaction'; | const action = viewerReacted ? 'remove_reaction' : 'add_reaction'; | ||||
sendReaction(reactionInput, action); | sendReaction(reactionInput, action); | ||||
}, | }, | ||||
[sendReaction, reactions], | [sendReaction, reactions], | ||||
); | ); | ||||
const emojiKeyboard = React.useMemo(() => { | const emojiKeyboard = React.useMemo(() => { | ||||
if (!renderEmojiKeyboard) { | if (!renderEmojiKeyboard) { | ||||
return null; | return null; | ||||
} | } | ||||
return <Picker data={data} onEmojiSelect={onEmojiSelect} />; | |||||
}, [onEmojiSelect, renderEmojiKeyboard]); | return ( | ||||
<div | |||||
ref={emojiKeyboardRef} | |||||
style={emojiKeyboardPositionStyle} | |||||
className={css.emojiKeyboard} | |||||
> | |||||
<Picker data={data} onEmojiSelect={onEmojiSelect} /> | |||||
</div> | |||||
); | |||||
}, [emojiKeyboardPositionStyle, onEmojiSelect, renderEmojiKeyboard]); | |||||
const messageTooltipContainerStyle = React.useMemo(() => tooltipStyle, []); | const messageTooltipContainerStyle = React.useMemo(() => tooltipStyle, []); | ||||
const containerClassName = classNames({ | const containerClassName = classNames({ | ||||
[css.container]: true, | |||||
[css.containerLeftAlign]: alignment === 'left', | |||||
[css.containerCenterAlign]: alignment === 'center', | |||||
}); | |||||
const messageTooltipContainerClassNames = classNames({ | |||||
[css.messageTooltipContainer]: true, | [css.messageTooltipContainer]: true, | ||||
[css.leftTooltipAlign]: alignment === 'left', | [css.leftTooltipAlign]: alignment === 'left', | ||||
[css.centerTooltipAlign]: alignment === 'center', | [css.centerTooltipAlign]: alignment === 'center', | ||||
[css.rightTooltipAlign]: alignment === 'right', | [css.rightTooltipAlign]: alignment === 'right', | ||||
}); | }); | ||||
return ( | return ( | ||||
<div className={containerClassName}> | <> | ||||
{emojiKeyboard} | {emojiKeyboard} | ||||
<div | <div className={containerClassName} style={messageTooltipContainerStyle}> | ||||
className={messageTooltipContainerClassNames} | |||||
style={messageTooltipContainerStyle} | |||||
> | |||||
<div style={messageTooltipTopLabelStyle}>{tooltipLabel}</div> | <div style={messageTooltipTopLabelStyle}>{tooltipLabel}</div> | ||||
{tooltipButtons} | {tooltipButtons} | ||||
{tooltipTimestamp} | {tooltipTimestamp} | ||||
</div> | </div> | ||||
</div> | </> | ||||
); | ); | ||||
} | } | ||||
export default MessageTooltip; | export default MessageTooltip; |
Nit: renderEmojiKeyboard should probably be named shouldRenderEmojiKeyboard. There's a naming convention in React where functions with names starting with "render" typically return React.Node