Skip to content

refactor: add opts and logic to allow submit on enter only under cuesheet line cell for time inputs #1945

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.

Sign up for GitHub

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

Closed
Shobhit-Nagpal wants to merge 2 commits into from
Closed

refactor: add opts and logic to allow submit on enter only under cuesheet line cell for time inputs #1945

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6865e7
refactor: add opts and logic to allow submit on enter only under cues…
Shobhit-Nagpal Jan 15, 2026
de5e2e7
refactor: remove check for same value
Shobhit-Nagpal Jan 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions apps/client/src/common/components/input/text-input/useReactiveTextInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export default function useReactiveTextInput(
onCancelUpdate?: () => void;
allowSubmitSameValue?: boolean;
allowKeyboardNavigation?: boolean;
allowSubmitOnEnterOnly?: boolean;
},
): UseReactiveTextInputReturn {
const [text, setText] = useState<string>(initialText);
Expand Down Expand Up @@ -50,10 +51,17 @@ export default function useReactiveTextInput(
* @param {string} valueToSubmit
*/
const handleSubmit = useCallback(
(valueToSubmit: string) => {
(valueToSubmit: string, triggeredKey?: string) => {
// No need to update if it hasn't changed
if (valueToSubmit === initialText && !options?.allowSubmitSameValue) {
options?.onCancelUpdate?.();
} else if (
valueToSubmit === initialText &&
options?.allowSubmitOnEnterOnly &&
triggeredKey !== 'Enter' &&
triggeredKey !== 'mod + Enter'
) {
Comment thread
Shobhit-Nagpal marked this conversation as resolved.
options?.onCancelUpdate?.();

@alex-Arc alex-Arc Jan 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Shobhit-Nagpal Shobhit-Nagpal Jan 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Shobhit-Nagpal Shobhit-Nagpal Jan 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 checkForSameValue and 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

@alex-Arc alex-Arc Jan 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

@Shobhit-Nagpal Shobhit-Nagpal Jan 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alex-Arc alex-Arc Jan 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Shobhit-Nagpal Shobhit-Nagpal Jan 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alex-Arc alex-Arc Jan 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Shobhit-Nagpal Shobhit-Nagpal Jan 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
const cleanVal = valueToSubmit.trim();
submitCallback(cleanVal);
Expand Down Expand Up @@ -105,7 +113,7 @@ export default function useReactiveTextInput(
'Enter',
() => {
isKeyboardSubmitting.current = true;
handleSubmit(text);
handleSubmit(text, 'Enter');
// clear flag after blur has been processed
setTimeout(() => {
isKeyboardSubmitting.current = false;
Expand All @@ -119,7 +127,7 @@ export default function useReactiveTextInput(
'mod + Enter',
() => {
isKeyboardSubmitting.current = true;
handleSubmit(text);
handleSubmit(text, 'mod + Enter');
// clear flag after blur has been processed
setTimeout(() => {
isKeyboardSubmitting.current = false;
Expand Down
1 change: 1 addition & 0 deletions apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/SingleLineCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const SingleLineCell = forwardRef(
const { value, onChange, onBlur, onKeyDown } = useReactiveTextInput(initialValue, submitCallback, ref, {
allowSubmitSameValue,
allowKeyboardNavigation: true,
allowSubmitOnEnterOnly: true,
submitOnEnter: true, // single line should submit on enter
submitOnCtrlEnter: true,
onCancelUpdate: handleCancelUpdate,
Expand Down
Loading