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

@kcolton

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.

@n-riesco

@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

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

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?

@shannonlal

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

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

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

@n-riesco

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

@shannonlal

@shannonlal

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

DialectSelector.react.js SGTM

@n-riesco

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

@shannonlal

@shannonlal

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 😝 . How about: SQLOptions, ESIndicesOptions, ESDocsOptions?

OK Cool. I will tackle this soon. Should be pretty easy

@n-riesco

@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

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

ApacheDrillPreview.react.js
S3Preview.react.js
preview/SQLTable.react.js
4.SQLTable.react.js
Should we standardize these to React Components?
I also noticed that we have two SQLTable's. The one under the Preview seems to be used; however, the one under the root of components (app/components/SQLTable.react.js) does not seem to be referenced.

@nicolaskruchten

From #383 (comment) @nicolaskruchten wrote:

Super cool! I played with it a bit and just have one bit of feedback... When I drop my CSV file, I have to hit "connect" and then it switches to a view where it says "connected" greyed out and "edit credentials" active. I then have to hit "Query" to actually see/chart the data. This makes a lot of sense in a database context but less in a file context. Maybe a bit of text saying "Connection succeeded! Click here to go to query your data, or click here to edit your credentials/change file" ?

Navigation Menu

Repository navigation

Refactor Preview.react.js and Settings.react.js #362

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Refactor Preview.react.js and Settings.react.js #362

Description

Activity

shannonlal commented on Feb 15, 2018

n-riesco commented on Feb 15, 2018

shannonlal commented on Feb 16, 2018

n-riesco commented on Feb 16, 2018

shannonlal commented on Feb 16, 2018

n-riesco commented on Feb 19, 2018

shannonlal commented on Feb 21, 2018

n-riesco commented on Feb 21, 2018

shannonlal commented on Feb 22, 2018

n-riesco commented on Feb 23, 2018

shannonlal commented on Feb 24, 2018

shannonlal commented on Feb 25, 2018

shannonlal commented on Feb 25, 2018

n-riesco commented on Feb 27, 2018

n-riesco commented on Mar 1, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions