OutlineView style the list of functions#2854
Conversation
using select source function.
Codecov Report
@@ Coverage Diff @@
## master #2854 +/- ##
==========================================
+ Coverage 58.57% 58.69% +0.12%
==========================================
Files 63 63
Lines 2356 2380 +24
Branches 484 487 +3
==========================================
+ Hits 1380 1397 +17
- Misses 976 983 +7
Continue to review full report at Codecov.
|
| return functions | ||
| .filter(func => func.value != "anonymous") | ||
| .map(this.renderFunction); | ||
| .map(func => this.renderFunction(func)); |
There was a problem hiding this comment.
Does this change make any difference ?
There was a problem hiding this comment.
Either approach is fine. We're generally prefer function references when there are no paramsmap(this.renderFunction), but we don't lint for it and either is fine.
There was a problem hiding this comment.
yes, it binds the this value. I just took a look through a few of the other files though and I noticed that doing the binding in the constructor is more common... would that be a clearer way of writing it?
ie)
constructor(props) {
super(props);
this.renderFunction = this.renderFunction.bind(this)
}
jasonLaster
left a comment
There was a problem hiding this comment.
Couple comments, but looking great!
| } | ||
|
|
||
| selectItem(location) { | ||
| const selectedSourceId = this.props.selectedSource.get("id"); |
There was a problem hiding this comment.
generally we destructure our props so that it is clear what our this deps are in the func
const {selectedSource, selectSource} = this.props;
| return functions | ||
| .filter(func => func.value != "anonymous") | ||
| .map(this.renderFunction); | ||
| .map(func => this.renderFunction(func)); |
There was a problem hiding this comment.
Either approach is fine. We're generally prefer function references when there are no paramsmap(this.renderFunction), but we don't lint for it and either is fine.
| }; | ||
| const startLine = 12; | ||
|
|
||
| symbolDeclarations.functions[0] = { |
There was a problem hiding this comment.
perhaps we also have a generateSymbolDeclaration factory as well so we can have the defaults in there
| location: generateFuncLocation(startLine) | ||
| }; | ||
|
|
||
| component.setProps({ sourceText, selectedSource }); |
There was a problem hiding this comment.
could we pass selectedSource in when we initially render the component?
Perhaps not based on how we're fetching the symbols, which is a shame i suppose. And if that's the case, we probably move symbols to a reducer so that we can have the symbols passed down alongside the source :) (we can do this in a new issue ofcourse)
- use object destructuring for props - create a generateSymbolDeclaration function - refactor the tests so that thre is a clear render with overrides function - remove aspects of the test that gave the wrong impression about how the code was working - add comments
|
| } | ||
|
|
||
| .outline-list__element { | ||
| color: blue; |
Associated Issue: #2562
Summary of Changes
This is the styling ticket for the OutlineList view, which introduces a function list for a given file
Test Plan
How to test:
this needs a few more things like integrations tests
Here's the Debugger's Testing doc
https://docs.google.com/document/d/1oBMRxV8A2ag2t22YsQOxTdEv0mXKzIg0tggJjRkU1S0/edit#.
Feel free to improve it!
Screenshots/Videos (OPTIONAL)