Uh oh!
There was an error while loading. Please reload this page.
Issue 362 options dropdown refactor - #387
Conversation
n-riesco
commented
Feb 26, 2018
I'll review this PR later tonight. But if you have the chance before, could change the PR, so that it doesn't touch |
| } | ||
| static propTypes = { | ||
| // See type of react-select's Select `value` property |
There was a problem hiding this comment.
I expected selectedTable and selectedIndex to be PropTypes.string (since we used them as keys).
Would PropTypes.string break anything?
| // See type of react-select's Select `value` property | ||
| selectedTable: PropTypes.any, | ||
| selectedIndex: PropTypes.any, | ||
There was a problem hiding this comment.
I think this line was here to make clear the above comment only applies to selectedTable and selectedIndex.
| this.renderElasticsearchDocs = this.renderElasticsearchDocs.bind(this); | ||
| } | ||
| renderSQLOptions() { |
There was a problem hiding this comment.
Since this code has moved to <SQLOptions>, let's get rid of this method.
| />); | ||
| } | ||
| renderElasticsearchIndecies() { |
There was a problem hiding this comment.
Since this code has moved to <ESIndicesOptions>, let's get rid of this method.
| />); | ||
| } | ||
| renderElasticsearchDocs() { |
There was a problem hiding this comment.
Since this code has moved to <ESDocsOptions>, let's get rid of this method.
| export default class ESDocsOptions extends Component { | ||
| static propTypes = { | ||
| selectedTable: PropTypes.any, |
There was a problem hiding this comment.
If we can't use PropTypes.string here, then we should bring back the comment // See type of react-select's Select 'value' property.
| static propTypes = { | ||
| elasticsearchMappingsRequest: PropTypes.object, | ||
| setIndex: PropTypes.func, | ||
| selectedIndex: PropTypes.any |
There was a problem hiding this comment.
If we can't use PropTypes.string here, then we should bring back the comment // See type of react-select's Select 'value' property.
| export default class SQLOptions extends Component { | ||
| static propTypes = { | ||
| selectedTable: PropTypes.any, |
There was a problem hiding this comment.
If we can't use PropTypes.string here, then we should bring back the comment // See type of react-select's Select 'value' property.
n-riesco
commented
Feb 26, 2018
I'd like new files to be lowercase. How about |
n-riesco
commented
Feb 26, 2018
shannonlal
commented
Feb 27, 2018
Merge branch 'upstream-master' into Issue-362-OptionsDropdown-Refactor
shannonlal
commented
Mar 3, 2018
@n-riesco I fixed most of your comments and removed the yarn.lock from the commit. Let me know if you have any additional comments or if you want to just merge this in. Thanks |
n-riesco
commented
Mar 3, 2018
💃 could you do a
|
shannonlal
commented
Mar 3, 2018
Thanks |
This is an initial refactoring of the options dialog. I have broken the options dialog into 3 different components. I have not done any refactoring/optimizing of the code at this point. In the next PR I will start adding Jest test for the different components. I am trying to keep the PR's small so I did not add any additional tests at this point.
@n-riesco If you have few minutes would you mind doing a quick review of this code. I welcome any comments or suggestions.
Thanks