Currently, if the user wants to edit a message which overflows (near the top or bottom of the ChatMessageList), the edit box may also overflow.
This diff addresses that, if the message edit box may overflow, we first scroll so that the edited message is in the center, and then open the edit box.
Details
Details
Run the app on Chrome, and checked:
- entered edit mode for a message which overflows, checked if the browser scrolled to it, and opened the edit box,
- entered edit mode for a message in the middle, checked if the edit box was opened instantly and without scrolling,
- added new lines for the edited message, and checked if it still fits in the container.
Videos:
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- kuba/message-editing-web
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 225 ↗ | (On Diff #27260) | I would change this name to something that suggests that this has something to do with edits. Like isMessageEditWindowOverflowed |
Comment Actions
Just some nits, not a full review. This one is pretty complicated, so would appreciate a detailed review by one of the listed reviewers!
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 72–74 ↗ | (On Diff #27260) | If we don't care about the return type of a prop, we should use mixed |
| 212 ↗ | (On Diff #27260) | Use mixed when return type doesn't matter |
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 72 ↗ | (On Diff #27714) | I don't think this is used anywhere? |
| 217–220 ↗ | (On Diff #27714) | Shouldn't this be called when the message edit window is NOT overflowed? |
| 221–222 ↗ | (On Diff #27714) | Shouldn't these be in reverse order? Can't the scrollIntoView finish before the state is set and it will result in callback not being called? Or actually shouldn't scrollIntoView be set in a callback in setState to ensure the state is set before we scroll? |
| 247 ↗ | (On Diff #27714) | Maybe return messageBottom > containerBottom || messageTop < containerTop so that the function name makes sense? |
| web/utils/tooltip-action-utils.js | ||
| 236–238 ↗ | (On Diff #27714) | |
Comment Actions
After a discussion with @inka we decided to add dynamic height calculation. Now, the maximum height of the edit box will be adjusted to the container height. We will scroll to the edited message only when there is insufficient space for the 150px text area in the edit box.
Video showing how it works:
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 224–225 ↗ | (On Diff #27724) | This is the same as what scrollingEndCallbackWrapper returns, can we not duplicate this? |
| 228 ↗ | (On Diff #27724) | Is this still necessary? |
| 238 ↗ | (On Diff #27724) | I feel like this might be confusing to other people who will look at this file. Can we write a short comment on why this is here? |
| 283 ↗ | (On Diff #27724) | Shouldn't this use the value from getMaxEditTextAreaHeight? |
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 283 ↗ | (On Diff #27724) | I don't think so. Currently, the idea behind this is:
The getMaxEditTextAreaHeight function is to determine the actual maximum TextArea height in two cases:
|
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 317–318 | We need to disable overflow-anchor for browsers that support it. Otherwise, when the edit box appeared, it jumped outside the message list container. | |
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 237 | ||
Comment Actions
High-level approach makes sense, but it's unfortunate that we're complicating a component that is already too complicated. Feedback below should be relatively easy to address
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 348 ↗ | (On Diff #28135) | You are introducing a lot of complicated code to a component that is already complicated. There should be extra attention paid to readability. If somebody reads onScroll on its own (without reading the whole component), they will not understand what you are doing here. Can you please rename this function to explain what it is doing? Eg. debounceEditModeAfterScrollToMessage or something |
| 351 ↗ | (On Diff #28135) | You appear to be implementing debounce from scratch. Can you instead use the _debounce utility from Lodash that we use elsewhere in the codebase? |
| web/chat/composed-message.react.js | ||
| 195 ↗ | (On Diff #28135) | I think we should use a more specific id, eg. ComposedMessageBox${messageKey(item.messageInfo)} or something |
| web/chat/edit-message-provider.js | ||
| 145–159 ↗ | (On Diff #28135) | setState can take a function, which allows us to avoid regenerating these every time scrollToMessageCallbacks changes |
| web/chat/edit-text-message.react.js | ||
| 31–32 ↗ | (On Diff #28135) | You are creating a contract for developers to maintain these numbers. It is very likely this contract will be broken in the future, as it's not documented and not discoverable. Ideally we can enforce that these numbers are correct somehow, by using them directly where the margins / height is defined. If you are sure we cannot do that, then the next best option is to add code comments at both locations to make sure that if somebody changes one place, they know that they need to change the other. |
Comment Actions
Address review
| web/chat/edit-text-message.react.js | ||
|---|---|---|
| 31–32 ↗ | (On Diff #28135) | The problem here is that this height is the sum of:
I can force the bottom height to be exactly 22px, but forcing the overall height might be cumbersome, especially, because it is used only for calculations. I can either:
For now, I went for a combination of two (enforced bottom row height, and added comments for the rest), but if needed I can change it. |
| 31–32 ↗ | (On Diff #28135) | I found out that I added the 8px padding two times:  I removed the redundant div.inputBarTextInput from the edit-text-message.react.js:  |
Comment Actions
Thanks for the update! This is really close, but going to request changes one last time due to the number of comments.
The biggest feedback is to fix Fast Refresh by moving the non-React exports to a separate file from the React exports. Two candidate files might be web/chat/chat-constants.js or web/chat/message-list-types.js, or you can create a new one if you'd like.
| web/chat/chat-input-text-area.react.js | ||
|---|---|---|
| 18 ↗ | (On Diff #28356) | It breaks Fast Refresh in React Native when you export a constant from the same file as a React component – see here:
Sorry I didn't point this out earlier! |
| web/chat/chat-message-list.react.js | ||
| 6 ↗ | (On Diff #28356) | We have a convention in our codebase to import only the parts of Lodash that we need: import _debounce from 'lodash/debounce.js'; |
| web/chat/composed-message.react.js | ||
| 30–34 ↗ | (On Diff #28356) | I am not sure, but I think this will break Fast Refresh too since it's a "non-React component" being exported from a file that also exports a React component. Can you move it out to another file? |
| web/chat/edit-text-message.react.js | ||
| 31–32 ↗ | (On Diff #28135) | Thanks for explaining! This seems reasonable to me |
| 32–33 ↗ | (On Diff #28356) | Same feedback about exports and Fast Refresh |
| 157 ↗ | (On Diff #28356) | The object being passed to style will get recreated on every render. Instead, we can define it just once at the top-level scope |
Comment Actions
The biggest feedback is to fix Fast Refresh by moving the non-React exports to a separate file from the React exports. Two candidate files might be web/chat/chat-constants.js or web/chat/message-list-types.js, or you can create a new one if you'd like.
I think there are more places where this issue is present. I created a task https://linear.app/comm/issue/ENG-4309/search-for-places-where-fast-refresh-is-broken-and-fix-it to fix them.
Comment Actions
Looks good, but I want to make sure that constants are kept in sync with CSS. I don't see any comments.
Would normally request changes here, but I really want to unblock @kuba, so I will accept. @kuba, please make sure that you address the inline comments! I'm guessing you need to add some additional code comments.
| web/chat/chat-constants.js | ||
|---|---|---|
| 40–45 ↗ | (On Diff #28392) | Do any of these constants need to be kept "in sync" with some place in CSS (or otherwise)? If yes, can you:
|
| web/chat/chat-message-list.react.js | ||
| 34 ↗ | (On Diff #28392) | This constant appears to only be used in this file. Can you share some context on why it's defined elsewhere, instead of being defined here? If the constant needs to be kept in sync with some location, we should add a comments on both sides explaining that. |
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 49 ↗ | (On Diff #28431) | Can you clarify whether or not this constant needs to be kept in sync with some other location, such as CSS? If so, please do two things:
|
| web/components/button.css | ||
| 12–13 ↗ | (On Diff #28431) | Please keep all lines to a max of 80 chars |
Comment Actions
Fix comments
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 49 ↗ | (On Diff #28431) | The idea behind editBoxTopMargin is to how much space should we leave between the top of the edit box (with maximum height), and the top of the container (screenshot). In that way, it has a 10px margin, so it looks much better.  I believe that it doesn't need to be kept in sync with any CSS. |
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 49 ↗ | (On Diff #28431) | Thanks for the clarification! Heads-up that the image doesn't seem to have successfully attached |
| web/chat/chat-message-list.react.js | ||
|---|---|---|
| 49 ↗ | (On Diff #28431) | I attached the image, hope it works now |