front: rename and refacto StdcmInputVia #10218
Conversation
clarani
requested review from
SarahBellaha,
SharglutDev and
Yohh
and removed request for
SarahBellaha
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #10218 +/- ##
==========================================
+ Coverage 81.43% 81.47% +0.03%
==========================================
Files 1057 1058 +1
Lines 104212 104272 +60
Branches 720 722 +2
==========================================
+ Hits 84870 84958 +88
+ Misses 19300 19273 -27
+ Partials 42 41 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Clara Ni <[email protected]>
- use useDebounce hook instead of custom debounce function - add a useEffect to update the store when the input is changed - move the callback function inside the component stopDurationInput Signed-off-by: Clara Ni <[email protected]>
clarani
force-pushed
the
cni/refacto-stdcm-input-via
branch
from
d400c63 to
5112724
Compare
| if (pathStep.stopType === StdcmStopTypes.PASSAGE_TIME) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I think it would be better to don't let the component decide to render or not but keep it in the parent to prevent unnecessary mounting
| onChange={(e) => { | ||
| // TODO: Find a better way to prevent user from entering decimal values | ||
| const value = e.target.value.replace(/[\D.,]/g, ''); | ||
| setStopDuration(value); | ||
| }} |
There was a problem hiding this comment.
Maybe it would be better to don't automatically replace value with a regex and warn the user that the value can't be used.
Because if I put 3.5, it compute 35 and if i don't pay attention, i will launch the simulation using a 35 minutes stop
There was a problem hiding this comment.
So you can make a handleChange function, that will turn a flag and display a warning if the value don't respect the expexted value
There was a problem hiding this comment.
Another solution would be to round up or down the value to keep minutes only, but this need to be discuss
There was a problem hiding this comment.
This portion of code is not related to my PR, can we discuss this later ? :/
We won't forget it since there is already a TODO comment above the problematic lines
There was a problem hiding this comment.
Following Mathieu's invitation, here is my user experience comment. This behaviour is problematic as the change is difficult to spot (a little larger letter spacing and a dot) and could affect results.
That said, we haven't witnessed users entering decimal values during our tests.
As there would be no way for us to know if users get this problem or not, I'm inviting POs to prioritize the issue as high, especially since we currently don't have the possibility to cancel computations.
There was a problem hiding this comment.
The value is now rounded. It's a quick fix, this will need to be discussed again with the POs
There was a problem hiding this comment.
especially since we currently don't have the possibility to cancel computations.
We can since a few weeks :)
No description provided.