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

Various quality fixes on network-map-viewer #64

Open
flo-dup opened this issue Dec 18, 2023 · 2 comments
Open

Various quality fixes on network-map-viewer #64

flo-dup opened this issue Dec 18, 2023 · 2 comments

Comments

@flo-dup
Copy link
Contributor

flo-dup commented Dec 18, 2023

Comments posted by @capyq in powsybl/powsybl-incubator#267 which were on the code extracted from https://github.com/gridsuite/gridstudy-app
This should be cut later in pieces to separate the easy parts from the most difficult ones (like typescript).

Originally posted in powsybl/powsybl-incubator#267 (comment)

I notice to much confusion between "null" and "undefined".
it's not a javaScript way to handle Object by using the class Map. it's better to use JSON Object.

Originally posted in powsybl/powsybl-incubator#267 (comment)

let labelDistanceInSegment = remainingDistance;
if (goodSegment.idx === 0) {
    labelDistanceInSegment = -alreadyDoneDistance;
} 

Originally posted in powsybl/powsybl-incubator#267 (comment)

return equipments ?? [];

Originally posted in powsybl/powsybl-incubator#267 (comment)

if (!equipementsToIndex?.length) {

Originally posted in powsybl/powsybl-incubator#267 (comment)

if (substationAdded || voltageLevelAdded) {

it's more readable

Originally posted in powsybl/powsybl-incubator#267 (comment)

why doing this at the end ?
this is cost expensive and if it's needed, you should add an else statement on the fullrender check (line 108)

Originally posted in powsybl/powsybl-incubator#267 (comment)

this should be a "const" you never asign it after this line

Originally posted in powsybl/powsybl-incubator#267 (comment)

does "null" and undefined are separate ? if not, you better use this version

if (!props.network && !props.geoData) {

Originally posted in powsybl/powsybl-incubator#267 (comment)

does null and undefined are two expected value ? if not, prefer short version

    !props.network && !props.geoData && !props.filteredNominalVoltages
) {

Originally posted in powsybl/powsybl-incubator#267 (comment)

separate in two lines we don't understand what should not be undefined

Originally posted in powsybl/powsybl-incubator#267 (comment)

this method is never used

Originally posted in powsybl/powsybl-incubator#267 (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 ?

Originally posted in powsybl/powsybl-incubator#267 (comment)

those are not else if but only if and you don't need else case because you return each statement

Originally posted in powsybl/powsybl-incubator#267 (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.

@flo-dup flo-dup transferred this issue from powsybl/powsybl-incubator Apr 15, 2024
@flo-dup
Copy link
Contributor Author

flo-dup commented Oct 23, 2024

As it was solved from #94, removed from description:

Originally posted in powsybl/powsybl-incubator#267 (comment)

you shouldn't use tabWith : 4 with the linewith 80.
so many line goes to 2 lines or more

@flo-dup
Copy link
Contributor Author

flo-dup commented Nov 28, 2024

As it was solved from #123, remove from description

Globaly, it would be better to switch to typescript it will reveal a lot of bug.

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

No branches or pull requests

1 participant