Skip to content

Migrate GUI to Typescript + React + ES6#143

Open
zls29 wants to merge 6 commits into
testfrom
typescript-react
Open

Migrate GUI to Typescript + React + ES6#143
zls29 wants to merge 6 commits into
testfrom
typescript-react

Conversation

@zls29

@zls29 zls29 commented Mar 5, 2017

Copy link
Copy Markdown
Collaborator

📖 Summary

Migrating the GUI to Typescript + React + ES6.

🔍 Details

To use more modern design practices, the GUI is being migrated to a component based architecture (via React).

Typescript is being used to create tighter contracts with the backend, hopefully reducing the likelihood of changes to server dto's silently breaking the GUI.

ES6 is being used to get some nice modern features (arrow syntax)...

Some modifications to the initial design were made (see the third commit) for the sake of performance. They were necessary to avoid rendering the entire component tree with every status message (several times per second).

Installation/Setup

cd public/flightControl
npm install

✔️ Testing

  • Simulate
    • All drones
    • Only 1 nonzero drone
  • Real Life
    • All drones
    • Only 1 nonzero drone

✔️ Verification

  • Checked for memory leaks

<link async rel="stylesheet" href="/node_modules/roboto-fontface/css/roboto/roboto-fontface.css">
<link async rel="stylesheet" href="node_modules/bootstrap/dist/css/bootstrap.min.css">
<link async rel="stylesheet" href="node_modules/material-design-lite/material.min.css">
<link async rel="stylesheet" href="node_modules/bootstrap-material-design/dist/css/ripples.min.css">

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmz38 I was getting a failure in loading styles related to this:
screen shot 2017-03-04 at 11 26 31 pm

Not really breaking anything, I can look at fixing it later.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this and the style other style issues.

super(props);
}

public shouldComponentUpdate(nextProps: ButtonProps): boolean {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are to avoid unnecessary re-rendering of large chunks of the DOM.

Comment thread Makefile Outdated
rm -f config/gazebo/models/x340/x340.sdf
cd build; cmake ..; make
cd build; cmake ..; make;
cd public/flightControl; webpack;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ./node_modules/.bin/webpack to avoid having to install it globally

Comment thread src/core/messaging.cpp


void send_message(const json &msg) {
void send_message(const string type, const json &msg) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this better? Having this seems to just add more redundant code to node/socket.js

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was more desirable than defining listeners for a generic msg type and relying on the json for a specific type, but I can do it that way if you prefer.

Comment thread src/gcs.cpp Outdated
jsonPosition.push_back(vehicles[i]->state.position.y());
jsonPosition.push_back(vehicles[i]->state.position.z());
jsonVehicle["position"] = jsonPosition;
jsonVehicle["position"] = {

@dennisss dennisss Mar 5, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any benefit in this change? Sending vectors as 3-tuples is pretty standard. (Changing it for the purpose of typescript is not a good reason)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I'll put it back to the way it was (we aren't using the data at the moment anyway).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using it on my branch for the viewer for wanted to avoid conflicts

value: string
}

export class FileSelector extends React.Component<FileSelectProps, FileSelectState> {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO if the react component doesn't really do anything but wrap an input, you probably don't need an extra component. But, either way, this component does not need to have it's own state. Use getDefaultProps instead. Having the state also prevents validation by the parent.

…steners when components are unbound, and other CR feedback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants