-
Notifications
You must be signed in to change notification settings - Fork 46
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
front: fix slopes editor form validation #5581
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #5581 +/- ##
============================================
- Coverage 19.45% 19.45% -0.01%
Complexity 2326 2326
============================================
Files 907 907
Lines 106891 106911 +20
Branches 2579 2579
============================================
Hits 20795 20795
- Misses 84584 84604 +20
Partials 1512 1512
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cf04158
to
f4b8f9d
Compare
f4b8f9d
to
ec3cda6
Compare
ec3cda6
to
296a08b
Compare
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.
Good job for the catch of this customOnChange !
Tested in local and everything seems to be working as expected. Tried also the forms for signals, detectors, switches, buffer stops and seen nothing strange.
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.
LGTM ✅
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 need to wait for Giuliana's point of view
b7b7a83
to
7e347db
Compare
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.
Take you for the changes. As checked with you, I think the curves and slopes values should be set to 0 as soon as we enter in the form.
Otherwise, I think it's strange that if the value is at 0, we see a -
when hovering the interval editor. It might mislead the user as it's the same behavior than the undefined
one.
Also you have some warnings in the file
7e347db
to
770d97a
Compare
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.
Thank you for the changes.
When I try to create a new tracksection, after drawing it and fill the required inputs, doesn't work. Its saying that curves and slopes are required properties even though gradiant and radius are set to 0 when entering the form.
On the other hand, the behavior when editing a track section with unspecified gradiants or radius is great, they are set automatically to 0, good job !
2626b06
to
0124cd5
Compare
c2d9163
to
091fb50
Compare
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.
Great job for this PR, thank you ! Tested and everything works as expected.
Left some naming comments. As I will be away the next 2 business days, I'll let @clarani resolve those so you are not blocked to merge the PR.
091fb50
to
8598ceb
Compare
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.
Thanks for this PR, almost there :)
544b021
to
2976e4c
Compare
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.
LGTM and tested ! Nice job, thank you very much 🙏 🎉
make slopes and curves value optional improve chart
2976e4c
to
355f023
Compare
close #3479
No value

Zero

