eclipsesource / eclipsesource/cdtcloud-deploymentserver

Review Feedback

Open
#101 1 comment 1 reaction 0 assignees View on GitHub
Dominant language
TypeScript
Stars
4
Forks
4
PR merge metrics
No merged PRs in 30d

Description

Hi guys,

I took a look at your code, here are my remarks.

- add readme files to each sub package
- add useful extensions to the list of recommended extensions

### deployment-server-ui
#### redux
- co-locate all redux stuff in one folder (reducers are next to the app folder which only contains the store)
- check whether your redux state is enough, especially the boolean flags like loading, maybe rather use a status with 3 states: 'loading', 'failed', 'successful'
- maybe use more of the redux/toolkit classes eg entities and thunks for retrieving the dashboard data: see https://redux-toolkit.js.org/api/createAsyncThunk, you could get rid of the `setError` and `setData` reducers that feel weird
- use optional instead of null
- maybe move data loaded in components into redux? thus also handle states etc there

#### generic
- remove commented out code
- double check class and interface naming, there is no need to prefix interfaces with `I`, some types are lowercase
- use push api on backend instead of pulling every 1-3 seconds
- move the code to App.tsx and directly use the Dashboard for the `/` path ?
- why use an identity function (defineFunctionalComponent) to wrap the functional component?
- split big components in smaller one eg the dashboard
- use optional chaining (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining)
- use arrow functions (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions)
- use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment to directly access the properties you later need of an object
- maybe pull out common formatter functions to a common util
- rename 'services' to 'hooks' as those are react hooks

### device-connector
- why the `#` prefix in private members

### grpc
- why the partial copy?

### theia-extension
- why the empty registerMenu registration?
- why the mix of got and axios in backend?

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.