fix problems introduced in text search #3640
Conversation
* fixes issues introduced by firefox-devtools#3636
Codecov Report
@@ Coverage Diff @@
## master #3640 +/- ##
=======================================
Coverage 54.51% 54.51%
=======================================
Files 120 120
Lines 4786 4786
Branches 992 992
=======================================
Hits 2609 2609
Misses 2177 2177
Continue to review full report at Codecov.
|
| {matches} | ||
| </span> | ||
| ); | ||
| return dom.span({ className: "line-value" }, ...matches); |
There was a problem hiding this comment.
Why did you need to change this back?
There was a problem hiding this comment.
You get this error when it's an array
return (
<span className="line-value">
{matches}
</span>
);
There was a problem hiding this comment.
Good question. There is an issue where <span>{matches}</span> causes a react
error that keys should be unique,.. oh! I see jasonLaster just commented about that :)
There was a problem hiding this comment.
I think we should be able to spread, but it doesn't work for me...
There was a problem hiding this comment.
looks like there are some issues with spreading and it is a performance hit for react as well.
babel/babylon#42
With that in mind, lets merge this and then refactor matches properly
Here's a gist of a patch i started https://gist.github.com/jasonLaster/11e46330c6cc17b36c4f78cb3229fa30
There was a problem hiding this comment.
Ah, interesting, so using spreads for children is off the table for now I guess.
Thanks for the links
| @@ -25,7 +25,7 @@ type Props = { | |||
| onExpand?: (item: any) => void, | |||
| onCollapse?: (item: any) => void, | |||
| renderItem: any, | |||
| disabledFocus: boolean | |||
| disabledFocus?: boolean | |||
There was a problem hiding this comment.
i don't know why this is needed now... but it looks good
There was a problem hiding this comment.
This was an attempt at making the prop optional to try and make the error mentioned in the PR
above go away. The disabledFocus prop is not used in src/components/PrimaryPanes/SourcesTree.js so it should be optional for now.
Unfortunately making it optional did not make yarn flow happy. I think this might be a known flow issue.
There was a problem hiding this comment.
my flow is happy. and so is CIs so you're off the hook
There was a problem hiding this comment.
Gah! So there is something strange in my setup, grrr.
| {matches} | ||
| </span> | ||
| ); | ||
| return dom.span({ className: "line-value" }, ...matches); |
There was a problem hiding this comment.
You get this error when it's an array
return (
<span className="line-value">
{matches}
</span>
);
convert ProjectSearch/*.js files to JSX #3636
Associated Issue: #3638
Note: There is a flow issue with this PR. Running
yarn flowgives this error:yarn flow v0.24.6 $ flow Error: src/components/PrimaryPanes/SourcesTree.js:250 v------------ 250: const tree = ManagedTree({ 251: key: isEmpty ? "empty" : "full", 252: getParent: item => { ...: 270: }); -^ function call 38: props: Props; ^^^^^ property `disabledFocus`. Property not found in. See: src/components/shared/ManagedTree.js:38 v 250: const tree = ManagedTree({ 251: key: isEmpty ? "empty" : "full", 252: getParent: item => { ...: 270: }); ^ object literal Found 1 error error Command failed with exit code 2.Which i cannot find a good solution to and I need some sleep. :-)