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

feat: Textbox trigger prop h2oai/q#1220 #11

Merged
merged 3 commits into from
Aug 21, 2020
Merged

feat: Textbox trigger prop h2oai/q#1220 #11

merged 3 commits into from
Aug 21, 2020

Conversation

mturoci
Copy link
Collaborator

@mturoci mturoci commented Aug 13, 2020

Added trigger prop to textbox component.

If the prop is set to true, qd.sync is called on change event. This was done in the same way as other components, but I am not sure if we really want to call it after each character typed. A better solution might be use blur event - gets called after losing focus off input.

@lo5 I am not sure what is the purpose / correct behavior for qd.sync. Right now, it simply resets the form - marks whole page as dirty. This (possible bug) can be seen also when using checkbox with trigger set. After clicking on checkbox, page rerenders, but state is not persisted.

Closes #37

@mturoci mturoci requested a review from lo5 as a code owner August 13, 2020 13:09
Copy link
Member

@lo5 lo5 left a comment

Choose a reason for hiding this comment

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

Thanks @mturoci - would be good to implement throttling so that the server doesn't go into hammertime :-)

ui/src/textbox.tsx Show resolved Hide resolved
py/examples/textbox.py Outdated Show resolved Hide resolved
@mturoci
Copy link
Collaborator Author

mturoci commented Aug 21, 2020

Thanks for the review @lo5! I have implemented debouncing for onChange in order to call sync after user stops typing. Throttling would send one sync per interval specified. Debounce will send sync after specified interval of no activity, which is better for our usecase. Let me know what you think.

I have also created piglatin translation example as you advised and rebased on master branch.

@lo5 I am not sure what is the purpose / correct behavior for qd.sync. Right now, it simply resets the form - marks whole page as dirty. This (possible bug) can be seen also when using checkbox with trigger set. After clicking on checkbox, page rerenders, but state is not persisted.

What about this behavior?

@lo5
Copy link
Member

lo5 commented Aug 21, 2020

Thanks, @mturoci. The trigger bug you observed is probably #44 - will fix tomorrow.

Copy link
Member

@lo5 lo5 left a comment

Choose a reason for hiding this comment

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

Thanks @mturoci!

@lo5 lo5 merged commit db6c609 into master Aug 21, 2020
@lo5 lo5 deleted the feat/issue-1220 branch August 21, 2020 17:16
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.

Add trigger attribute to ui.textbox()
2 participants