Using callback functions to change chosen button, accept it or close overlay.
Details
Details
Tested how it all fits together. Results on a video (currently without scrolling overlay content - added in next diff):
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| web/chat/chat-input-bar.react.js | ||
|---|---|---|
| 419–422 ↗ | (On Diff #20178) | I think the destructure syntax can be used here, but however you prefer. I find this to look cleaner |
Comment Actions
Destructure as Inka suggested.
| web/chat/chat-input-bar.react.js | ||
|---|---|---|
| 419–422 ↗ | (On Diff #20178) | Right, I haven't thought about using it in this context, thanks for suggestion! |
| web/chat/chat-input-bar.react.js | ||
|---|---|---|
| 425–451 | You can reduce duplication in this code by creating a map from key to function and using it instead of else if chain. Something like the included code, but it should be possible to simplify it even further. | |
| web/utils/typeahead-utils.js | ||
| 127 | ||
| 127–129 | Why only one prop has overlay in name? Do we need it? | |
| 129 | Can we keep it next to setChosenPositionInOverlay? | |
| 133–141 | It should work and doesn't need to be changed, but it would be less fragile if the logic was using keys instead of idx. | |
Comment Actions
Responded to inlines and fixed code.
Tested everything once again.
| web/chat/chat-input-bar.react.js | ||
|---|---|---|
| 425–451 | Thanks for the suggestion! I felt like it is really repetitive, but couldn't find a way to simplify it. | |
| web/utils/typeahead-utils.js | ||
| 127–129 | I forgot to rename chosenActionPosition argument. Renamed it so it match name from callsite and correspond to its setter. | |
| 133–141 | I'm not sure if we can use keys here. They are based on user id: key: suggestedUser.id, as you can see in getTypeaheadTooltipActions function above. And chosenPositionInOverlay is simply index in list which doesn't really correspond to user id. | |