-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
swap all use of get_event_triggers to rx.EventHandler props #3458
Conversation
@@ -223,35 +276,6 @@ def _get_imports(self): | |||
}, | |||
) | |||
|
|||
def get_event_triggers(self) -> Dict[str, Callable]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing this has resulted in the addition of the base Component.get_event_triggers default triggers, is that appropriate for this component? or should we have a get_event_triggers
here that returns an empty dict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding get_event_triggers
that returns {}
has caused events triggers to be completely removed from the docs on the data-editor page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we delete the EventTriggers
class now?
I'm not sure yet, they are still used in the base Component I think. I'm hesitant to convert this one to rx.EventHandler right now because I'm not sure it won't have side effect. if we can I'll do it in a follow up PR. |
what the title says.