-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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: [APPSMTH-22] Execute action on widget focus and blur #18128
Conversation
Welcome to the Appsmith community! Thank you for your first pull request and making this project better. 🤗 Please make sure that you raise a review request so your code change does not go unnoticed. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
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.
Lets use same helptext for onFocus and onClur across widgets.
onFocus - Triggers an action when the widget receives focus
onBlur - Triggers an action when the widget loses focus
@dilippitchika
@@ -294,6 +294,24 @@ class SingleSelectTreeWidget extends BaseWidget< | |||
isBindProperty: true, | |||
isTriggerProperty: true, | |||
}, | |||
{ | |||
helpText: "Triggers an action when a select field gets focus", | |||
propertyName: "onFocus", |
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.
should call this property onOpen instead of onFocus ?
@dilippitchika @somangshu
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.
@sbalaji1192 most of the frameworks out there are using focus / focused / onFocus. Seems like its a dev friendly word. But keeping appsmith in mind and all the target audience we have onOpen
might be a good option, open to explore
cc @dilippitchika @Nikhil-Nandagopal what do you think?
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.
onFocus and onOpen are 2 different things, let's not mix them. Let's keep the scope constrained to focus only for now and we will create a new issue for onOpen.
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.
@dilippitchika Though we are calling it onFocus, it gets triggered when the dropdown is opened. onBlur also gets called when dropdown is closed.
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.
Oh, you are right then it's a problem. OnFocus and OnBlur should only be triggered when the widgets are in/not in focus. Not when the dropdown is open or closed. The scenario where onBlur and dropdown close should happen together is when the focus itself is lost like user clicking outside the widget.
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.
Understood, looks like this is a limitation from the library itself and this needs to be updated. Let me test the preview and get back with further queries.
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.
@dilippitchika Will hold off review till you're back with queries if any
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.
@gitstart-appsmith Ok i checked, the problem i see with select widgets (select, multi-select etc) is the event triggers only when the popover is open or closed not when the widget is focused or blurred. Here's a loom - https://www.loom.com/share/a5447aae932441348fa0dd3456ea89c9
My expectation is the event onFocus should be triggered when we are focusing on the widget. If we are unable to do that we should rename this to onOpen and onClose. But i need to understand how to do onFocus and onBlur on these widgets.
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.
@dilippitchika, Here's a loom video explaining why our approach was used https://www.loom.com/share/6ce616c8900b4e9b8ad15ab9be8ea597
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.
Thanks for the explanation, @gitstart-appsmith. For now let's change the name of the onFocus and onBlur to onDropdownOpen and onDropdownClose for the following widgets. This should be fine for now
- Select
- Multi-select
- Multi-tree-select
- Tree-select
/ok-to-test sha=1085701 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3617908982. |
/ok-to-test sha=1085701 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3629175587. |
/ok-to-test sha=1085701 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3636363637. |
4667793
/ok-to-test sha=4667793 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3645875948. |
/ok-to-test sha=4667793 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3654343179. |
/ok-to-test sha=519ddbe |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3654846927. |
/ok-to-test sha=90782bc |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3654855829. |
Description
This PR enables onFocus and onBlur events on the following Widgets
NB: This would require updating the Appsmith Documentation
Fixes #6452
Type of change
How Has This Been Tested?
Checklist: