refactor: add opts and logic to allow submit on enter only under cuesheet line cell for time inputs#1945
Conversation
…heet line cell for time inputs
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| triggeredKey !== 'Enter' && | ||
| triggeredKey !== 'mod + Enter' | ||
| ) { | ||
| options?.onCancelUpdate?.(); |
There was a problem hiding this comment.
It seams that we don't provide a function here
https://github.com/Shobhit-Nagpal/ontime/blob/de5e2e79f9100eaa47f43724ee681b8495e9e21b/apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/cuesheetColsFactory.tsx#L189
witch result is potentially stale data
if you make a change and then press tab the edit will remain but it is not submitted
There was a problem hiding this comment.
i think this was being prevented with the check for initial value and the new value before. if there's a change in the value, it would not enter this statement block
even if we were to pass in the cancel callback, it should submit the value in case there's a change
There was a problem hiding this comment.
i can think of 2 ways to handle this.
- can bring the equality check of new value and initial value and add in a comment to mention why the check is present
- can add in another flag called
checkForSameValueand based on if this is true, then check if the value is same or not and proceed further (but this feels like a long way to do the same thing as above)
do you have any other suggestions on this? @alex-Arc
There was a problem hiding this comment.
first another question, what do we mean by options?.allowSubmitOnEnterOnly ?
what I read is that only the enter key is allowed to submit this field, that is also what the following checks suggest
options?.allowSubmitOnEnterOnly && triggeredKey !== 'Enter' && triggeredKey !== 'mod + Enter'
There was a problem hiding this comment.
allowSubmitOnEnterOnly means to only allow submission of the value if the user has pressed enter or mod + enter.
Suppose this flag is false, we're accepting submission on Escape and Tab as well. The triggeredKey is to be able to know which key was pressed when navigating the cuesheet to so we can confirm if the key is an Enter or mod + Enter key
The scenario would be that the user is active on a time input and they tab to the next input without changing any value, it would still submit. We want to allow a submission only on the enter key and not allow tab or escape to enter
So we pass in the flag of allowSubmitOnEnterOnly as true and check which key was triggered while navigating to then confirm if we want to submit the value or not
Do let me know if I can be more clear on a specific part
There was a problem hiding this comment.
good I understood correctly,
In that case the logic as it is now is correct, we just need to supply the onCancelUpdate so that it actually works
There was a problem hiding this comment.
yes, but the edge case you mentioned where if the user has changed the value and then they tab, the value won't be submitted is not what we want. in any case that the value changes, we want it to submit
There was a problem hiding this comment.
Then allowSubmitOnEnterOnly is not a good name
looking further back into it, is the issue not that we have some formatting in the time filed that automatically get a value when you tab into them?
should the fix perhaps be isolated to those inputs
There was a problem hiding this comment.
the issue is that when the value doesn't change on time inputs and the user tabs / escapes on the active time field, there's a submission of the value that happens
i did try scoping the issue to the time field inputs but the issue is that there's a prop of lockedValue which re-renders the component and it keeps changing. if not that, the input has an isEditing check on which component to display (input or div). this gets the component to lose its memory of what to do
basically, what we need to do is:
- check if the value is same or not
- if the user pressed escape / tab AND the value is same, then don't submit
- if the value is different, submit in either case
the reason this logic above isn't satisfied by the allowSubmitSameValue prop is because it's linked to the value being locked or not which keeps changing when we're active on the time inputs. to not change the current behavior and also be able to add this fix, added a flag to check for this condition. from what i've tried, scoping it to the input field itself might make the code more complicated and i'm not sure on where to start with that either
happy to take in suggestions or improvements on the name or the overall logic
Changes made
allowSubmitOnEnterOnlyfield under opts foruseReactiveTextInputhooktriggeredKeyunder handleSubmit to keep track of whether user pressed submit or nothandleSubmitwhich implements the behavior to cancel update unless user pressed enter or mod+enter to submit the same valueallowSubmitOnEnterOnlyoption as true under<SingleLineCell />under cuesheet-table-elementsWorking demo:
ontime-cuesheet-submit-on-enter-only-1-2026-01-15_22.16.31.mp4