Project Search (components)#3089
Conversation
codehag
left a comment
There was a problem hiding this comment.
Great work! a few comments on the progress so far
| } | ||
|
|
||
| function searchResults(sources: SourcesMap, query) { | ||
| if (false) { |
There was a problem hiding this comment.
this looks weird, because we will never enter this loop. should it be deleted, or updated to be meaningful?
| this.props.toggleProjectSearch(false); | ||
| } | ||
|
|
||
| renderFileSearch() { |
There was a problem hiding this comment.
should this be used in render? it looks like it has logic that was moved out of the render function
| font-family: monospace | ||
| } | ||
|
|
||
| .project-text-search .result .line-number { |
There was a problem hiding this comment.
this is getting pretty specific. maybe we could use BEM to keep this from going too far. maybe something like
.project-text-search__line-number. Also result seems like a bit of a dangerous css selector, as I can see it being used in a lot of places for different purposes.
There was a problem hiding this comment.
yeah - these are throw away styles. I think something BEM like would be better
| return dom.div( | ||
| { | ||
| className: "file-result", | ||
| style: {} |
There was a problem hiding this comment.
oops, i'll remove it, but these are throw away as i was quickly sketching
| itemHeight: 20, | ||
| autoExpand: 1, | ||
| autoExpandDepth: 1, | ||
| getParent: item => null, |
There was a problem hiding this comment.
do we need item here? it seems like getParent will always return null
|
|
||
| renderResults() { | ||
| const { results } = this.props; | ||
| return ManagedTree({ |
There was a problem hiding this comment.
this is very complex, it feels like most of these callbacks belong to ManagedTree. Maybe a job for a future refactoring
| } | ||
|
|
||
| setExpanded(item: ManagedTreeItem, isExpanded: boolean) { | ||
| setExpanded(item: Item, isExpanded: boolean) { |
| } | ||
|
|
||
| renderListItem(item: ResultListItem, index: number) { | ||
| const { selectItem, selected } = this.props; |
jasonLaster
left a comment
There was a problem hiding this comment.
a couple things are left in because the new component was sketched quickly. I assume someone will come in and clean it up
| placeholder: L10N.getStr("sourceSearch.search"), | ||
| size: "big" | ||
| TextSearch({ | ||
| sources |
There was a problem hiding this comment.
we should move this to a separate render function
| font-family: monospace | ||
| } | ||
|
|
||
| .project-text-search .result .line-number { |
There was a problem hiding this comment.
yeah - these are throw away styles. I think something BEM like would be better
| return dom.div( | ||
| { | ||
| className: "file-result", | ||
| style: {} |
There was a problem hiding this comment.
oops, i'll remove it, but these are throw away as i was quickly sketching
Codecov Report
@@ Coverage Diff @@
## master #3089 +/- ##
==========================================
+ Coverage 66.15% 66.33% +0.18%
==========================================
Files 74 75 +1
Lines 2603 2620 +17
Branches 527 528 +1
==========================================
+ Hits 1722 1738 +16
- Misses 881 882 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #3089 +/- ##
==========================================
- Coverage 66.67% 66.33% -0.35%
==========================================
Files 75 75
Lines 2677 2620 -57
Branches 542 528 -14
==========================================
- Hits 1785 1738 -47
+ Misses 892 882 -10
Continue to review full report at Codecov.
|
efb66b8 to
0b6cff6
Compare
Associated Issue: #1108
Summary of Changes