Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRefactor Preview.react.js and Settings.react.js #362
Comments
|
@n-riesco . I spent sometime working my way through the front end last night and I have some ideas on how to tackle this. I think one of the main things I would like to add is Jest to test the components. I think this will help document the expected behaviour and allow us to identify areas where maybe the application is crashing and we could add some basic error handling. I can start laying out a plan to do very small PRs where we would add a test file for each Component. This should be very easy to review and could be done in very small chunks. If this makes sense I will layout a proposed plan and get on this. Your thoughts? |
|
@shannonlal I'm OK with introducing jest. |
|
I did some more work tonight and I was able to get Jest working and running a simple test against one of the components. My recommendation would be to start with the basic non-connected React Components and build some basic tests around them. Once we get some basic test coverage we could tackle the some of the more complex components like Settings to get this to work. Here is a break down of the components I would like to tackle. I suggesting that we do a PR for each test to keep them simple for reviewing. Here are the components that I suggest we put some simple tests in place for
Question. Are we using any documenting tools (JSDocs) or anything like that? |
yes, please! :) While we're at it, I'd like to rethink the naming of some of these components. For example, some of the points I'd like to address:
Since I'm working on #350 and #351, I'll be changing
The code is sparely documented. I'd suggest that we do like in https://github.com/plotly/plotly.js . We don't explicitly use JSDoc, but when we document new functions, we prefer to do so using JSDoc's format. |
|
For any developers who are interested in helping out with this. We are looking at going through our front end code to add some testing with Jest. If you have any experience with React and Jest and would like to help out please drop me a line here and I can help get you setup. |
|
@shannonlal After reviewing your PR #378 and writing the bug fix #379, I've noticed that the code to connect is split between Since you're already familiar with This will move us a step closer to |
|
@n-riesco. Yes this makes sense. I might move onto some of the other smaller components first to see if there are other areas that might need some code refactoring as well. I think my short term goal should be to get some basic unit testing across the different components. So when we start doing some refactoring we at least have some tests which hopefully make changes easier. After the ConnectButton is done I was planning on moving over to the Dialog Selector. Your thoughts? |
|
|
@n-riesco As I am working my through the list, I think I will tackle the Logger next. I had a quick look at the OptionsDropdown and I wanted to get your thoughts on breaking this into 3 smaller components: SQL Options, ElasticSearch Indecies, and ElasticSearch Docs. I think we would still have the OptionsDropdown component but it would reference 3 other components (SQL Options, ElasticsearchIndeciesOptions, and ElasticSearchDocsOptions). Your thoughts? |
Yes, I'm OK with the split. I wouldn't spend a lot of time on it, but I'd take the chance to fix Falcon's bad orthography |
|
OK Cool. I will tackle this soon. Should be pretty easy |
|
@n-riesco Is it me or is app/components/Settings/Logger.react.js and LoggerController.react.js not being used. I did a quick search and can't see to find any references to it? Am I missing something? Thanks |
|
@n-riesco Just working my way through some of the other components and noticed some of them extend React's component but others do not. Should we be standardizing? As an example the following do not extend Component:
|
|
From #383 (comment) @nicolaskruchten wrote:
See also #383 (comment) |
|
@shannonlal I've dealt with |
cc/ @kcolton @chriddyp @tarzzz
I'm starting this discussion prompted by issues:
Although the code editor lives in CodeEditorField.react.js and the schema preview in TableTree.react.js, it's very likely that we'll have to touch code in their parent components Preview.react.js and Settings.react.js.
I'd like to use this issue to discuss any significant changes to Preview.react.js and Settings.react.js before creating a PR.