Refactor Preview.react.js and Settings.react.js

[n-riesco]

cc/ @kcolton @chriddyp @tarzzz

I'm starting this discussion prompted by issues:

• Typing in the SQL editor should not be laggy from heavy repaints of the chart #360 (poor performance of code editor)
• Duplicated sql-schemas requests #361 (duplicated sql-schemas requests)

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.

[shannonlal]

@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?

[n-riesco]

@shannonlal I'm OK with introducing jest.
@chriddyp @tarzzz Are you OK with using jest to test out React components?

[shannonlal]

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

• /app/components/Settings/ConnectButton/ConnectButton.react.js
• /app/components/Settings/DialectSelector/DialectSelector.react.js
• /app/components/Settings/Logger/Logger.react.js
• /app/components/Settings/Logger/LoggerController.react.js
• /app/components/Settings/OptionsDropdown/OptionsDropdown.react.js
• /app/components/Preview/components/AxisDropZone.react.js
• /app/components/Preview/components/Box.react.js
• /app/components/Preview/ApacheDrillPreview.react.js
• /app/components/Preview/CodeEditorField react.js
• /app/components/Preview/S3Preview.react.js
• /app/components/Preview/SQLTable.react.js
• /app/components/Preview/TableTree.react.js
• /app/components/Tabs/Tab.react.js
• /app/components/Tabs/Tabs.react.js

Question. Are we using any documenting tools (JSDocs) or anything like that?

[n-riesco]

@shannonlal

I suggesting that we do a PR for each test to keep them simple for reviewing.

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:

• when users report an issue with TableTree, they refer to that component as DB overview. I'd like the name of the component to reflect this. For example (this is just an idea), instead of having Preview/TableTree.react.js, I'd have query/view/database.jsx.

• prefer short names (and avoid stuttering); e.g.:

  • instead of Preview/S3Preview.react.js, I'd have query/view/s3.jsx;
  • instead of Preview/ApacheDrillPreview.react.js, I'd have query/view/apache-drill.jsx
  • instead of Preview/CodeEditorField.react.js, I'd have query/editor.jsx

---

Since I'm working on #350 and #351, I'll be changing SQLTable.react.js and Preview/ChartEditor.react.js, no need to test those until I'm done with the changes.

---

Question. Are we using any documenting tools (JSDocs) or anything like that?

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.

[shannonlal]

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.

[n-riesco]

@shannonlal After reviewing your PR #378 and writing the bug fix #379, I've noticed that the code to connect is split between ConnectButton and renderSettingsForm. This not only makes debugging more difficult, but also coding (I guess this is the reason why Falcon has two different buttons to connect and to edit the connection).

Since you're already familiar with ConnectButton, after merging #378, how do you feel about moving renderSettingsForm, renderEditButton and ConnectButton into one component that renders the Connection panel?

This will move us a step closer to Settings being just a container that coordinates all the panels, i.e.: Connection, Query, Plotly and F.A.Q.

[shannonlal]

@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]

@shannonlal

After the ConnectButton is done I was planning on moving over to the Dialog Selector. Your thoughts?

DialectSelector.react.js SGTM

[shannonlal]

@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?