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

Auto-Increment and Error Messaging for Duplicate Contingency List Names in Script #277

Closed
wants to merge 9 commits into from

Conversation

achour94
Copy link
Contributor

@achour94 achour94 commented Sep 4, 2023

Implement Automatic Incrementation and Error Message for existing filters contingency list Name when Copied to Script

…ters contingency list Name when Copied to Script

Signed-off-by: achour94 <berrahmaachour@gmail.com>
…ters contingency list Name when Copied to Script

Signed-off-by: achour94 <berrahmaachour@gmail.com>
@souissimai souissimai self-requested a review September 4, 2023 14:44
if (initialValue !== '') {
updateFormState(initialValue);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Collaborator

Choose a reason for hiding this comment

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

forbidden in our code base :)

@achour94 achour94 requested a review from jonenst September 5, 2023 13:49
@SlimaneAmar SlimaneAmar self-requested a review September 5, 2023 15:05
Copy link
Contributor

@souissimai souissimai left a comment

Choose a reason for hiding this comment

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

Code Ok
Test Ok

useEffect(() => {
if (initialValue !== '') {
updateFormState(initialValue);
setValue(initialValue);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this effect run multiple times ? If not, there's not need for setValue as initialValue is passed to the useState() ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bad name "initialValue" if it triggers additional behaviors when it changes. In react the convention is to only use the value for the first render

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, it willl trigger the useeffect twice in the case we will get the name from the backend

<CircularProgress />
) : (
<NameWrapper
initialValue={generatedName}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would fix the initialValue behavior and restore the value={...} and onChange{...} prop

open,
onClose,
onClick,
currentName,
title,
}) => {
const [newNameValue, setNewNameValue] = React.useState(currentName);
const [generatedName, setGeneratedName] = useState('');
const [newNameValue, setNewNameValue] = useState(currentName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we do use generatedName and newNameValue
Maybe one is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because in that case (initialValue={newNameValue}) the useEffect with initialValue as a dependency will be triggered each time newNameValue is changed

@flomillot flomillot self-requested a review September 7, 2023 07:42
@achour94 achour94 closed this Sep 19, 2023
@achour94 achour94 deleted the increment_alias_copy_name branch September 19, 2023 14:11
@achour94
Copy link
Contributor Author

replaced with #285

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.

4 participants