Skip to content
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

Add an on_enter handler for input type components #296

Merged
merged 1 commit into from
May 28, 2024

Conversation

orangerd
Copy link
Contributor

Add an on_enter handler for <input type="text"> type components, so that input boxes can be "submitted" using the keyboard. Currently, there's no handler for "Enter", so input boxes are usually "submitted" using a relevant button hooked to a handler.

Copy link
Collaborator

@wwwillchen wwwillchen left a comment

Choose a reason for hiding this comment

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

Overall - looks good, if you could add a test, that would be great. thanks.

mesop/components/input/e2e/input_app.py Outdated Show resolved Hide resolved
@orangerd orangerd force-pushed the lewisden_20240521_input_enter_fix branch 2 times, most recently from 14a6035 to 8933fd3 Compare May 24, 2024 22:26
@wwwillchen
Copy link
Collaborator

wwwillchen commented May 26, 2024

I originally wrote a comment suggesting an alternative approach of on_key_up and having a KeyUpEvent which would be more generic than on_enter, but that becomes very similar to on_input and it makes it ambiguous whether on_key_up would be called before on_input, etc. I think keeping this simple as on_enter LGTM, eventually we'll want to have some kind of broader hotkey support (#283), but this seems like a good initial step.

@richard-to WDYT about the API here?

@richard-to
Copy link
Collaborator

richard-to commented May 26, 2024

I originally wrote a comment suggesting an alternative approach of on_key_up and having a KeyUpEvent which would be more generic than on_enter, but that becomes very similar to on_input and it makes it ambiguous whether on_key_up would be called before on_input, etc. I think keeping this simple as on_enter LGTM, eventually we'll want to have some kind of broader hotkey support (#283), but this seems like a good initial step.

@richard-to WDYT about the API here?

I was thinking that we'd have something more generic and similar to what's available on the Javascript side, which would be on key up. But that's a great point about on_input.

On key up and on input are not technically the same. It sort of feels like a on compromise of on key up/press/down in some ways.

I do wonder if it would be possible to pass in modifier keys and key presses like enter with on_input. For example, it's possible we could pass in a list of the modifier keys and key presses. I think that is possible, but due to the necessary debouncing and latency, I'm not sure how easy to use it would be from an API usage standpoint.

So do I think the proposed hot key idea could work more generically.


I think the on_enter is still very useful since it is a common use case. So eventually could be seen as a shortcut for whatever hot key implementation.

Maybe one suggestion feature-wise could be support the also common "Modifier" + Enter use case that Will mentioned. But I think that could be added on later to the current api if necessary.

@wwwillchen
Copy link
Collaborator

I originally wrote a comment suggesting an alternative approach of on_key_up and having a KeyUpEvent which would be more generic than on_enter, but that becomes very similar to on_input and it makes it ambiguous whether on_key_up would be called before on_input, etc. I think keeping this simple as on_enter LGTM, eventually we'll want to have some kind of broader hotkey support (#283), but this seems like a good initial step.
@richard-to WDYT about the API here?

I was thinking that we'd have something more generic and similar to what's available on the Javascript side, which would be on key up. But that's a great point about on_input.

On key up and on input are not technically the same. It sort of feels like a on compromise of on key up/press/down in some ways.

I do wonder if it would be possible to pass in modifier keys and key presses like enter with on_input. For example, it's possible we could pass in a list of the modifier keys and key presses. I think that is possible, but due to the necessary debouncing and latency, I'm not sure how easy to use it would be from an API usage standpoint.

So do I think the proposed hot key idea could work more generically.

I think the on_enter is still very useful since it is a common use case. So eventually could be seen as a shortcut for whatever hot key implementation.

Maybe one suggestion feature-wise could be support the also common "Modifier" + Enter use case that Will mentioned. But I think that could be added on later to the current api if necessary.

OK sounds good, let's go with this on_enter for now. We could add the modifier keys (e.g. ctrl, shift) to the EnterEvent to support the advanced use cases I was thinking about, but we can do that in a follow-up PR.

@wwwillchen
Copy link
Collaborator

@orangerd you'll need to fix a pre-commit issue. BTW you can install the pre-commit hooks to catch it locally, see: https://google.github.io/mesop/internal/development/#commit-hooks

@orangerd orangerd force-pushed the lewisden_20240521_input_enter_fix branch from 12d6d69 to 93a897d Compare May 28, 2024 16:19
Allow an on_enter handler for input types so that input boxes can be 'submitted' using the keyboard.
@orangerd orangerd force-pushed the lewisden_20240521_input_enter_fix branch from 93a897d to 9f8fb3c Compare May 28, 2024 16:21
@orangerd
Copy link
Contributor Author

Ah sorry - yes installed the pre-commits

@wwwillchen
Copy link
Collaborator

Thanks! Merging

@wwwillchen wwwillchen merged commit 60c788a into google:main May 28, 2024
3 checks passed
@orangerd orangerd deleted the lewisden_20240521_input_enter_fix branch June 4, 2024 19:39
wwwillchen pushed a commit to wwwillchen/mesop that referenced this pull request Jun 14, 2024
Allow an on_enter handler for input types so that input boxes can be 'submitted' using the keyboard.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants