Desktop: Fix missed highlighting when using the global search #3717
Conversation
| // If there is a currently active search, it's important to re-search the text as the user | ||
| // types. However this is slow for performance so we ONLY want it to happen when there is | ||
| // a search | ||
| const textChanged = props.searchMarkers.keywords.length > 0 && props.content !== previousContent; |
There was a problem hiding this comment.
If we need previousContent here, shouldn't it be in the dependency array?
There was a problem hiding this comment.
That's a good question. previousRenderBody wasn't in there before (hence why I didn't mirror it), and I think maybe it isn't needed.
previousContent only changes when props.content changes (although the change would happen after), because this effect is sensitive to props.content then the effect will trigger with the new props.content and the previous value.
If we add previousContent as a dependency wouldn't that just cause an extra trigger of the effect but it wouldn't have any effect (becuase previousContent now equals props.content)?
There was a problem hiding this comment.
In general I think it's best to have all the dependencies in the array, and then define clearly how it works within the effect (in your case, with the conditional that skip the effect in certain cases). Otherwise we rely on implied behaviour, which might be fine in a small effect, but can be harder to make sense of as it gets more complex.
If it causes an extra trigger that does nothing I think this is acceptable.
There was a problem hiding this comment.
Fair enough. I added previousContent and previousSearchMarkers as dependants and tested to confirm that they don't add additional searches (they don't).
|
@laurent22 suggested changes are made! Should be good to go. |
replaces #3622
This solves 3 issues that were present with searching.