feat: match HOC display names during search #360
Conversation
There was a problem hiding this comment.
This feature seems like it would benefit from a couple of new tests 😄
react-devtools-experimental/src/__tests__/treeContext-test.js
Lines 268 to 269 in 644c9c4
After trying the feature, I also think that maybe some form of visual results highlighting is going to be important after all, to avoid confusion
Maybe something like below? I can implement this part though. 😄Not sure how to (efficiently) detect if the match is inside of the tag or not off the top of my head.
.
| hocDisplayNames.length > 0 | ||
| ) { | ||
| if (hocDisplayNames.some(name => regExp.test(name) === true) === true) { | ||
| pushResult(); |
There was a problem hiding this comment.
Seems like we could simplify this a bit and remove the overhead of an extra function call by using an if/else. What do you think?
function recursivelySearchTree(
store: Store,
elementID: number,
regExp: RegExp,
searchResults: Array<number>
): void {
const { children, displayName, hocDisplayNames } = ((store.getElementByID(
elementID
): any): Element);
if (
displayName != null &&
regExp.test(displayName) === true
) {
searchResults.push(elementID);
} else if (
hocDisplayNames != null &&
hocDisplayNames.length > 0 &&
hocDisplayNames.some(name => regExp.test(name))
) {
searchResults.push(elementID);
}
children.forEach(childID =>
recursivelySearchTree(store, childID, regExp, searchResults)
);
}
I was wanting to add tests anyway. I’ll get on them. :D
Yeah, I felt the same about this. It’s certainly not clear why an element is being returned as a match.
I like this a lot! Solves the problem I was hitting with trying to show the user the match is from a tag that was being truncated. I’d be happy to look into adding this as well. |
|
I've simplified the match logic and added/updated related tests. I can add the highlighting tonight or I can add it as a follow-up PR if you like. |
There was a problem hiding this comment.
Cool. This looks good to me. We can follow up on the HOC highlighting in a future PR. Visual style is pretty straight forward, but it's going to need a little thought on how to most efficiently determine which part of the element (name, visible HOC name, or +number) to highlight.
|
One thing I notice now is that maybe we should special case HOCs like forward ref and memo so the search matches them too, because it seems a little weird that you can search for e.g. "withFoo" but not "Memo" when they look the same. |
|
Thanks for contributing by the way! 😁 |
I would image there is just a preference system where the preferred match is the one that is surfaced. I would also suggest that we match on all so that the behavior of the system is very explicit, however that approach is slightly misleading as the actual match algorithm is preference based for quick matching. This is probably worth a ticket you can assign to me, if you like.
Oh! This is a very good idea, probably worth a ticket if you want to assign it to me.
My pleasure! I'm a big fan of this project. :D |
|
Follow up work briefly detailed in #365 |
closes #299
Enables the user to use the component search capability against HOC display names.