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

Visualize composite modifications #538

Merged
merged 26 commits into from
Nov 13, 2024

Conversation

Mathieu-Deharbe
Copy link
Contributor

Signed-off-by: Mathieu DEHARBE <[email protected]>
Copy link
Contributor

@etiennehomer etiennehomer left a comment

Choose a reason for hiding this comment

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

Later we will need the whole NetworkModificationTreePane in commons-ui... To be discussed if it's ok to put it in this lib. Or let's create a lib for big business components ?

{modifications && (
<List sx={unscrollableDialogStyles.scrollableContent}>
{modifications.map((modification: NetworkModificationMetadata) => (
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

There are missing keys.
<React.Fragment key={imodification.uuid}> ?
And remove it from ListItem ?

Copy link
Contributor

Choose a reason for hiding this comment

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

and then you have to remove the key from the ListItem as the React.Fragment managing the key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, done.

return null;
}
return intl.formatMessage(
{ id: 'network_modifications.' + modif.type },
Copy link
Contributor

Choose a reason for hiding this comment

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

network_modifications.${modif.type}

Copy link
Contributor Author

@Mathieu-Deharbe Mathieu-Deharbe Oct 30, 2024

Choose a reason for hiding this comment

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

Yes done. eslint is forcing me to do it now anyway !

...computeLabel(modif),
}
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

const labelData = {
        ...modif,
        ...computeLabel(modif),
    };

    return intl.formatMessage(
        { id: `network_modifications.${modif.type}` },
        labelData
    );
    };

{modifications && (
<List sx={unscrollableDialogStyles.scrollableContent}>
{modifications.map((modification: NetworkModificationMetadata) => (
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

and then you have to remove the key from the ListItem as the React.Fragment managing the key

);
};

const renderNetworkModificationsList = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

render.. could be improved, it doesn't clearly convey what the function does
maybe (generate or create)NetworkModificationsList or something else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a copy paste. Maybe the original author used "render" because it contains jsx anchor code ?
Anyway I prefer generate so ok.

broadcastChannel: BroadcastChannel;
}

export const CompositeModificationEditionDialog: FunctionComponent<CompositeModificationEditionDialogProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

only Edition ? it could be just CompositeModificationDialog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. It should be enough, at least for now. And I am tired of those huge names.

.catch((errorMessage) => {
snackError({
messageTxt: errorMessage,
headerId: 'contingencyListEditingError',
Copy link
Contributor

Choose a reason for hiding this comment

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

contingencyListEditingError ?
it is copy paste forgetten

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops ! Well seen thank you. Done.

import { AppState } from 'redux/reducer';
import Box from '@mui/material/Box';

const CompositeModificationEditionForm = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

CompositeModificationForm ?

<CompositeModificationEditionDialog
open={true}
titleId={'compositeModification'}
// @ts-expect-error TODO: manage null case(s) here
Copy link
Contributor

Choose a reason for hiding this comment

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

fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a very simple and straightforward way. If it ever happens it will cause a regular error during the fetch.

I didn't change the other TODOs on the same subject that are in the same switch.

@@ -11,6 +11,10 @@ export const FilterType = {
EXPERT: { id: 'EXPERT', label: 'filter.expert' },
};

export const NetworkModificationType = {
COMPOSITE: { id: 'COMPOSITE_MODIFICATION', label: 'compositeModification' },
Copy link
Contributor

Choose a reason for hiding this comment

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

compositeNetworkModification

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted to "MODIFICATION"

/**
* Get the basic data of the network modifications contained in a composite modification
*/
export function getCompositeModificationContent(id: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

fetchNetworkCompositeModification

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK for fetch but this is the content of the composite modification that we are fetching, not the composite modification itself, which we already have.

@Mathieu-Deharbe
Copy link
Contributor Author

Later we will need the whole NetworkModificationTreePane in commons-ui... To be discussed if it's ok to put it in this lib. Or let's create a lib for big business components ?

Yes we will need to migrate this as well.
The discussion about whether or not commons-ui should in the end contain everything... is a big debate.

Signed-off-by: Mathieu DEHARBE <[email protected]>
@@ -145,7 +147,8 @@
"download.message.studies": "studies",
"download.message.others": "others",
"download.error": "An error occurred while downloading element {elementName}",
"MODIFICATION": "Composite modification",
"MODIFICATION": "Network modification",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok but you can check with PO.
And you can change the key with "networkModification"

"editContingencyList": "Editer la liste d'aléas",
"editFilter": "Editer le filtre",
"editContingencyList": "Éditer la liste d'aléas",
"editFilter": "Éditer le filtre",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok to keep it. But for lisibility of PR, tracability of the code, it's always harmful to change things that have nothing to do with the PR's subject

Copy link

@Mathieu-Deharbe Mathieu-Deharbe merged commit ce0e79a into main Nov 13, 2024
4 checks passed
@Mathieu-Deharbe Mathieu-Deharbe deleted the visualize-composite-modifications branch November 13, 2024 10:26
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