-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Bugfix/7873 time conductor input validation #7886
Conversation
validate on change because input is too aggressive
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7886 +/- ##
==========================================
+ Coverage 57.58% 64.91% +7.33%
==========================================
Files 676 434 -242
Lines 27353 13592 -13761
Branches 2685 0 -2685
==========================================
- Hits 15750 8823 -6927
+ Misses 11267 4769 -6498
+ Partials 336 0 -336
see 369 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
Small change but looks solid
Tested this locally and it looks really good, very nice work. I noticed a very minor niggle that could be fixed in a followup. Invalid time bounds result in the submit button being disabled which is great. However an invalid combination of dates leaves the submit button enabled. If you try and click the enabled submit button it shows a validation error but for consistency it would be great if invalid date bounds also disabled the submit button, like invalid times do eg. minor.TC.bug.mov |
This is by design. The point is to not punish the user early. Give them a chance to input valid bounds, and if they don't, prevent them from submitting invalid bounds. This comes into play when changing both start and end bounds. The user almost always edits the start bounds first, and then is punished too early if say they are moving the start bounds and end bounds forward in time to a time window that is after the end bounds. I think this is what the video is showing. If not, we can re-examine. |
remove unnecessary code paths
@akhenry , @shefalijoshi , addressed your comments in this commit, 8d4d57d. ready for re-review |
@davetsay Love it, the validation is subtle and brilliant. For all intents and purposes it's art. |
Closes #7873
Describe your changes:
input
eventchange
event, so user is not punished earlyAll Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist