Network map viewer built with Vite #267
Conversation
|
Globaly, it would be better to switch to typescript it will reveal a lot of bug. |
| substation.voltageLevels.find((v) => | ||
| props.filteredNominalVoltages.includes( | ||
| v.nominalVoltage | ||
| ) | ||
| ) !== undefined |
There was a problem hiding this comment.
separate in two lines we don't understand what should not be undefined
| [useName] | ||
| ); | ||
|
|
||
| const getUseNameParameterKey = useCallback(() => { |
| if (infos != null) { | ||
| const name = infos.name; | ||
| return useName && name != null && name.trim() !== '' | ||
| ? name | ||
| : infos?.id; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
you are mixing null and undefined
1 - if infos is undefined you also should return null (or undefined) => if (!infos){...}
2 - why testing infos?.id but not infos.name ?
| if (nominalVoltage >= 300) { | ||
| return [255, 0, 0]; | ||
| } else if (nominalVoltage >= 170 && nominalVoltage < 300) { | ||
| return [34, 139, 34]; | ||
| } else if (nominalVoltage >= 120 && nominalVoltage < 170) { | ||
| return [1, 175, 175]; | ||
| } else if (nominalVoltage >= 70 && nominalVoltage < 120) { | ||
| return [204, 85, 0]; | ||
| } else if (nominalVoltage >= 50 && nominalVoltage < 70) { | ||
| return [160, 32, 240]; | ||
| } else if (nominalVoltage >= 30 && nominalVoltage < 50) { | ||
| return [255, 130, 144]; | ||
| } else { | ||
| return [171, 175, 40]; | ||
| } |
There was a problem hiding this comment.
those are not else if but only if and you don't need else case because you return each statement
network-map-viewer/.prettierrc.json
Outdated
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "tabWidth": 4, | |||
There was a problem hiding this comment.
you shouldn't use tabWith : 4 with the linewith 80.
so many line goes to 2 lines or more
| @@ -457,16 +426,14 @@ const NetworkMap = (props) => { | |||
| }; | |||
|
|
|||
| const renderOverlay = () => ( | |||
There was a problem hiding this comment.
It's not a good practice to use variable to store component React doesn't tag this as react component and re-render it more than needed.
|
@capyq, could you confirm that all the comments you wrote about the packaging were answered by @CBiasuzzi ? So that we can merge this pull request, or modify further on those aspects if the changes are not satisfying. For the other comments, I'll create an issue in this repository, but I'd prefer that we do not touch the gridsuite code at all before they integrate the viewer, I mean before gridsuite/gridstudy-app#1323 is merged. Indeed any changes here in the viewer code would be annoying when updating the viewer with latest changes from gridsuite. Or we should apply all those changes both on the viewer here and on the gridsuite repository, which also doubles the work. |
Ok for me. |
Signed-off-by: Christian Biasuzzi <christian.biasuzzi@soft.it>
Signed-off-by: Christian Biasuzzi <christian.biasuzzi@soft.it>
…uilt with vite; removes unused dependencies Signed-off-by: Christian Biasuzzi <christian.biasuzzi@soft.it>
Signed-off-by: Christian Biasuzzi <christian.biasuzzi@soft.it>
flo-dup
force-pushed
the
network_map_viewer_vitejs
branch
from
4589e35
to
69aa8d5
Compare
|
|
As mentioned above, the remaining comments are on the code extracted from github.com/gridsuite/gridstudy-app and will be considered when the extracted component will be integrated there. An issue (#268) has been created to track down those comments. |
This reverts commit f535583. Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
Alternative build tool used for the network-map-viewer component.
Same context as the one in PR #263
(please refer to it for details).
The component's code has not changed, with respect to the branch referenced in PR #263
The difference lies in the building phase, for which Vite is used, instead of Webpack
(compared to Webpack Vite offers some advantages, such as simpler configurations).