search

plotly / falcon

Free, open-source SQL client for Windows and Mac 🦅
https://plot.ly/free-sql-client-download/
MIT License
5.13k stars 279 forks source link

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

Open n-riesco opened 6 years ago

n-riesco commented 6 years ago

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.

shannonlal commented 6 years ago

@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 commented 6 years ago

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

shannonlal commented 6 years ago

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?

n-riesco commented 6 years ago

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

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@shannonlal

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

DialectSelector.react.js SGTM

shannonlal commented 6 years ago

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

n-riesco commented 6 years ago

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

shannonlal commented 6 years ago

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

shannonlal commented 6 years ago

@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

shannonlal commented 6 years ago

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

  1. ApacheDrillPreview.react.js

  2. S3Preview.react.js

  3. preview/SQLTable.react.js 4.SQLTable.react.js

  4. Should we standardize these to React Components?

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

n-riesco commented 6 years ago

From https://github.com/plotly/falcon-sql-client/issues/383#issuecomment-368991461 @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" ?

See also https://github.com/plotly/falcon-sql-client/issues/383#issuecomment-369039871

n-riesco commented 6 years ago

@shannonlal I've dealt with SQLTable in #393