Scan-o-Matic / scanomatic

Scanomatic
GNU General Public License v3.0
10 stars 4 forks source link

Duration input #358

Closed local-minimum closed 6 years ago

local-minimum commented 6 years ago

I tested that it worked with the scan-jobs ui manually too.

codecov-io commented 6 years ago

Codecov Report

Merging #358 into master will increase coverage by 9.91%. The diff coverage is 98.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #358      +/-   ##
==========================================
+ Coverage   40.01%   49.93%   +9.91%     
==========================================
  Files         192      231      +39     
  Lines       16635    19917    +3282     
  Branches     2894     2902       +8     
==========================================
+ Hits         6657     9945    +3288     
+ Misses       9570     9543      -27     
- Partials      408      429      +21
Impacted Files Coverage Δ
...r_data/js/src/containers/ScanningRootContainer.jsx 100% <ø> (ø) :arrow_up:
scanomatic/ui_server_data/js/src/api.js 84.21% <ø> (ø) :arrow_up:
...i_server_data/js/src/components/NewScanningJob.jsx 77.77% <ø> (ø) :arrow_up:
scanomatic/ui_server_data/js/src/api.spec.js 100% <ø> (ø)
...ver_data/js/src/components/NewScanningJob.spec.jsx 100% <100%> (ø)
...js/src/containers/NewScanningJobContainer.spec.jsx 98.91% <100%> (ø)
scanomatic/ui_server_data/js/src/Duration.js 100% <100%> (ø) :arrow_up:
...data/js/src/containers/NewScanningJobContainer.jsx 100% <100%> (ø) :arrow_up:
scanomatic/ui_server_data/js/src/Duration.spec.js 100% <100%> (ø)
...rver_data/js/src/components/DurationInput.spec.jsx 100% <100%> (ø)
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 190d16e...e907db7. Read the comment docs.

gdetrez commented 6 years ago

i'm not completely sure this completely fixes the issue. If I recall correctly, using the little arrow on a number input produce a change event but no focus/blur. If that's correct, the overwritten value will be left in the state even though it probably shouldn't. One way to fix it is to handle the focus event and handle a change differently depending on whether the field is focused or not.

Since this is non trivial, maybe add a few more complex scenario in the tests. E.g.

local-minimum commented 6 years ago

Added more complex tests. Perhaps not all you suggested because some I didn't fully grasp.

gdetrez commented 6 years ago

Looks good enough for me. We can probably iron out corner cases later. :+1: