Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite of the frontend in React #44

Open
wants to merge 125 commits into
base: master
from
Open

Rewrite of the frontend in React #44

wants to merge 125 commits into from

Conversation

@angustrau
Copy link
Member

angustrau commented Jan 12, 2020

Description

This is a complete rewrite of the frontend in React. I'll be interested to know if there are any parts of the code that are hard to understand and need clarification, especially if you're not too experienced with the framework.

A rundown of the improvements

  • Uses the React framework for better code structure, maintainability, and development speed
  • Uses modern Javascript syntax with strict linting
  • Has client-side routing for "instant" page loads
  • All the code is now documented
  • The production code is minified for faster load times
  • Fixed up some security bugs

Screenshots

image
image
image
image
image
image
image
image
image

Steps to Test

Instructions to run the dev server are in the README.

@angustrau angustrau removed the WIP label Jan 13, 2020
@angustrau

This comment has been minimized.

Copy link
Member Author

angustrau commented Jan 13, 2020

The react branch is feature complete with master now. Barring any review changes, I'm calling this PR done.

Copy link
Member

harsilspatel left a comment

i'm in loveeee with this!!!
can't appreciate it enough @angustrau <3

client/public/robots.txt Show resolved Hide resolved
Copy link
Member

khanguslee left a comment

Please do (way) shorter PRs next time. But nonetheless, amazing work! Glad this was done finally! Left some questions and fixes for you to work with! Before you move on with anything else, I definitely need unit tests for everything here. Feel free to do one page at a time or setup github actions before so we can get some automated testing!

I will need to run this on my mac, so expect some more comments later on!

client/src/App.js Outdated Show resolved Hide resolved
client/src/api/v2/powerPlan/index.js Show resolved Hide resolved
client/src/api/v2/sensors/status.js Show resolved Hide resolved
client/src/api/v2/sensors/data.js Show resolved Hide resolved
*/
export default function FontAwesomeIcon({ icon }) {
const [width, height] = icon.icon;
const svgPathData = icon.icon[4];

This comment has been minimized.

Copy link
@khanguslee

khanguslee Jan 15, 2020

Member

I feel like this will break with other icons? Pls confirm?

This comment has been minimized.

Copy link
@angustrau

angustrau Jan 15, 2020

Author Member

This will work with all fontawesome icons through NPM. Their data structure is just weird. I rolled my own component since the official react bindings include 30kB of polyfills.

This comment has been minimized.

Copy link
@khanguslee

khanguslee Jan 16, 2020

Member

Yeah that's fair enough. Totally understand why you did this. Leave a comment somehow referring to this problem? I think another developer reading this line will be confused here.

}, [primarySelected, secondarySelected, setPrimaryActive, setSecondaryActive]);

// Only render controls when overlay data is available
const primary = primaryOverlays ? (

This comment has been minimized.

Copy link
@khanguslee

khanguslee Jan 15, 2020

Member

Rename to primaryControls

</Col>
) : null;

const secondary = secondaryOverlays ? (

This comment has been minimized.

Copy link
@khanguslee

khanguslee Jan 15, 2020

Member

Rename to secondaryControls

</Col>
</Row>
{!textMode ? (
<div>

This comment has been minimized.

Copy link
@khanguslee

khanguslee Jan 15, 2020

Member

I would put the below components into a function above called renderGraphicalDashboard or similar. Makes it easier to read this return function. Would also have a renderTextDashboard too for the other path.

</InputGroup.Append>
</InputGroup>
</Form>
{distanceSet ? (

This comment has been minimized.

Copy link
@khanguslee

khanguslee Jan 15, 2020

Member

Use short-circuiting instead.

!!distanceSet && <Alert variant="success">Distance sent!</Alert>
Make sure power model is not running before you start it!
</Alert>
{successAlert ? (
<Alert variant="success">

This comment has been minimized.

Copy link
@khanguslee

khanguslee Jan 15, 2020

Member

Short circuit this! See above example.

@angustrau angustrau added the WIP label Jan 15, 2020
@angustrau angustrau removed the WIP label Jan 18, 2020
@angustrau angustrau requested a review from khanguslee Jan 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.