-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat(area): move area command creation #2322
base: dev
Are you sure you want to change the base?
Conversation
… into feat/move-area-command-creation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add unit test for this command as we only have one currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be nice to add a unit test on the command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I encountered a "bug" which was already present before your dev. When updating the color of an area inside the layer1, it also brings back its position to the layer 0 position. I don't think we should do that.
We should investigate and if we can change this, fix it and add a test inside the test_area.py file you introduced
…-creation # Conflicts: # antarest/study/storage/variantstudy/business/command_reverter.py
We can merge this. I will create a ticket adressing the bug that might comes from the front |
set: t.Optional[t.List[str]] = None | ||
|
||
|
||
# review: is this class necessary? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it still wip ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the comment because it is still used. Thanks for pointing out
add a command to move an area on the map. Replace multiple UpdateConfig commands