Details
Please watch the demo video to see the changes I made:
Testing a window that changes screen size:
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
If there is such a strong reason, you should absolutely expect your reviewer will call you out, and you should preemptively annotate and explain why are you doing this.
Noted, I will be more explicit about these things moving forward.
First concern was just me not being careful, and I will make sure I take a deeper look with git diff also moving forward
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 135 | I added this exception because I was getting this error in my editor when I added emojiKeyboardRef.current to the the list of dependencies. We should be updating this memo whenever emojiKeyboardRef.current changes so that we can measure the dimensions of the emoji keyboard  | |
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 135 | This error was a good thing as it highlighted a bigger issue. Changing the value of ref won't result in render and in recomputing the memo. It's even explicitly included in the error message! That's the difference between state and refs. | |
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 135 | Okay sounds like I need to read up more on this, thanks for pointing this out! | |
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 55–60 ↗ | (On Diff #22867) | Do we need this null check? If that's not necessary, we can (probably) avoid using callback and use setter directly <div
ref={setEmojiKeyboardNode}
style={emojiKeyboardPositionStyle}
className={css.emojiKeyboard}
> |
| 134–140 ↗ | (On Diff #22867) | Initially emojiKeyboardNode might be null - do we handle this correctly in getEmojiKeyboardPosition? |
| 134–140 ↗ | (On Diff #22867) | This can be simplified |
| 135 | Great! | |
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 134–140 ↗ | (On Diff #22867) | Yup there are default values in getEmojiKeyboardPosition that are equal to the expected values of the width and height for the emoji keyboard in case emojiKeyboardNode is null |
Rerequesting review since made some changes with the ref logic by switching from useCallback hook => useRef hook
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 54–63 ↗ | (On Diff #22942) | It 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. |
@ginsu, strongly encourage you to be more critical of yourself before submitting code in the future. This pattern of submitting code you feel like might be "good enough" to sneak past review leads to wasted time for both you and your reviewers. You should hold yourself to a higher standard – it will help you learn better, and it will save you time.
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 54–63 ↗ | (On Diff #22942) |
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. |
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.
Love this idea!!
I spent some time reading through this and I understand how it works now. @ginsu wanted to catch the ref just as it was set, so he made a useEffect that would trigger immediately after renderEmojiKeyboard flips, which as it turns out will trigger the rendering of the emoji keyboard. He got this idea from the article he linked earlier.
The "sin" here is twofold:
- The coupling between the code rendering emojiKeyboard (how it returns null until renderEmojiKeyboard flips) and the effect triggering setEmojiKeyboardNode (which needs emojiKeyboard to render, so it waits for renderEmojiKeyboard to flip as well) is unclear, which hurts readability. The reader of the code doesn't understand why the effect mentions renderEmojiKeyboard at all in its dep list given that it's not used. Both @tomek and I had this issue on our first read of this code.
- In the future, if you must write code with readability issues like this, you should absolutely expect that it will confuse the reader, and add code comments (or at least diff comments) to make it clear.
- We're using an effect here to set state. That is generally consider an anti-pattern in React. It's an anti-pattern because it's bad for performance (effects only run after a render is completed, which leads to unnecessary render cycles), and it's bad for UX (user might see the screen "flash" an intermediate state before the effect runs). Sometimes it's unavoidable, but it's rare. React even advises calling setState from render instead of from an effect if possible.
Here is what we should do instead:
- The ref being passed to the emojiKeyboard div (emojiKeyboardRef) should be a function instead of a React.useRef.
- You can always pass a function as a ref= prop
- I think you can call setState from that function directly, which avoids the need for the effect.
- Based on reading your code today, I think you probably will only want to call setState if the node is truthy. (When the node unmounts, it will call the ref function again with a falsey value.)
- You should obviously wrap the whole function you pass as ref= with React.useCallback
- A separate improvement we can make is to replace the node state with emojiKeyboardPosition state.
- Basically you can make the ref function set emojiKeyboardPosition directly by calling getEmojiKeyboardPosition.
- The ref function will need to include tooltipPositionStyle and tooltipSize in its dep list.
- It's generally best to avoid storing mutable objects (like DOM objects) in React state since reasoning about them changing is hard.
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 53 ↗ | (On Diff #22942) | 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 |
| 54–63 ↗ | (On Diff #22942) |
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. |
I attempted to address the feedback above, but still running into one issue where the node passed through the ref still has a height and width of zero. (I saw this by logging node.getBoundingClientRect() in emojiKeyboardRef
I believe the reason for this issue is that we are setting the ref of the container div before <Picker /> is fully "painted" onto the DOM. (To see what was going on, I temporarily removed the shouldRenderEmojiKeyboard condition in emojiKeyboard and saw that when I logged node.getBoundingClientRect(), the first log had a height and width of zero, but the subsequent logs had the correct values.)
I worked with @atul to find a solution to this, but we could not find anything promising. One of the solutions we looked into was passing the ref as a prop in <Picker />; however, we got the following error message below, and after looking at the docs and some related issues on Github, it seems we can't directly pass in a ref into <Picker />
https://github.com/missive/emoji-mart#options--props
https://github.com/missive/emoji-mart/issues/416
I did some more reading online, and some sources pointed me toward implementing a resize observer. I took a quick look into adding a resize observer, and I believe this would help the delayed mount issue we are facing but I also haven't installed and verified it.
https://github.com/ZeeCoder/use-resize-observer#readme
In terms of moving forward, I feel the best thing right now would be to hardcode the width and the height and land as is and create a follow-up task to address this. I know that the ideal solution here would be to compute the values of the width and height of the emoji keyboard; however, these values do not change, and I do feel like I am spending too much time trying to figure this out and should move on to other things like avatars.
- The rename would've been best in a separate diff
- Does D6632 need to be updated so that getEmojiKeyboardPosition no longer looks at emojiKeyboard.getBoundingClientRect()?
- It's not clear to me which change is causing the issue here. I think you should investigate:
- If getting rid of the useEffect is what caused the issue, then you could potentially bring back the useEffect. You would be able to address my "coupling" concern, but not my "set state in effect" concern. Still, that's better than nothing.
- It's possible that immediately calling getEmojiKeyboardPosition from the ref function is causing the problem, and you could potentially undo that by moving back to storing the node itself as state, and computing getEmojiKeyboardPosition in a useMemo that takes the node as input
Separately – I'd like to avoid spending any more of @atul's or your time investigating this one... I think it's probably leading to a lot of wasted cycles, and preventing you from focusing on avatars. As a next step here, either I can take a look and record a Loom of my investigation, or alternately we can try to pair on it. Let me know your preference
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 60 ↗ | (On Diff #23266) | Have you considered adding a sleep here and just waiting for the keyboard to draw on-screen? It seems bad but slightly better than hardcoding the width / height, which will likely cause a bug next time this library is updated |
Does D6632 need to be updated so that getEmojiKeyboardPosition no longer looks at emojiKeyboard.getBoundingClientRect()?
Yes if I had moved forward with the hardcoding option, I would have updated the diff in the stack before this which to make sure getEmojiKeyboardPosition doesn't look at emojiKeyboard.getBoundingClientRect()
If getting rid of the useEffect is what caused the issue, then you could potentially bring back the useEffect. You would be able to address my "coupling" concern, but not my "set state in effect" concern. Still, that's better than nothing.
Bringing back the effect solves this issue, but I was initially concerned about the "set state in effect", so I initially just ruled it out.
It's possible that immediately calling getEmojiKeyboardPosition from the ref function is causing the problem, and you could potentially undo that by moving back to storing the node itself as state, and computing getEmojiKeyboardPosition in a useMemo that takes the node as input
When I tried this, I would get the correct value of height, but width would still be zero
As a next step here, either I can take a look and record a Loom of my investigation, or alternately we can try to pair on it. Let me know your preference
Happy to do whatever is easier for you, I have no plans this evening so I can pair on it later tonight if you would like.
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 60 ↗ | (On Diff #23266) | I did try adding a sleep when I was tinkering around, and it does work with the emoji keyboard initially rendering in the incorrect position then after a split second rendering in the correct position. I thought that this was a pretty bad solution. |
Oh actually, it sounds like you can just try this:
Bringing back the effect solves this issue, but I was initially concerned about the "set state in effect", so I initially just ruled it out.
You've already done it before so I figure you actually don't need my help here! Hopefully you can put up one more revision and we can land it.
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 59–62 ↗ | (On Diff #23274) | I added a comment here to help with readability and better address @ashoat's coupling concerns. I also think this comment would be good to caution future devs who work in this file and see this code to not make this a recurring pattern in our codebase |
Okay, never mind... let's pause on this work until you have time to pair with me
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 59–62 ↗ | (On Diff #23274) | This comment is misleading... it implies that refs get passed in before a component renders to the DOM. This is not correct... the ref gets passed in after the component first renders. What's happening here is that <Picker /> has some custom behavior where it renders initially with height: 0 and width: 0, and then resizes itself |
| 67 ↗ | (On Diff #23274) |
|
| web/chat/message-tooltip.react.js | ||
|---|---|---|
| 59–62 ↗ | (On Diff #23274) | The real reason this is happening is because emoji-mart doesn't actually render itself until a React.useEffect here, which won't get run until after our ref is set. This is why we appear to need to use a React.useEffect ourselves as well. (The idiomatic thing for emoji-mart to do would've been to render itself immediately instead of in an effect... but if the effect is strictly necessary, at least they should have a callback so that the parent component can know when rendering is complete...) |