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: fired events section added #276

Merged
merged 6 commits into from
Mar 6, 2025
Merged

feat: fired events section added #276

merged 6 commits into from
Mar 6, 2025

Conversation

yanaminkova
Copy link
Member

Fired events section is added in the Events tab.

@yanaminkova yanaminkova requested a review from IlianaB January 24, 2025 12:58
@IlianaB
Copy link
Member

IlianaB commented Jan 29, 2025

https://sapui5untested.int.sap.eu2.hana.ondemand.com/resources/sap/ui/documentation/sdk/index.html?sap-ui-xx-sample-id=sap.uxap.sample.ObjectPageOnJSON&sap-ui-xx-sample-lib=sap.uxap&sap-ui-xx-sample-origin=.&sap-ui-xx-dk-origin=https%3A%2F%2Fsapui5untested.int.sap.eu2.hana.ondemand.com&sap-ui-theme=sap_horizon&sap-ui-rtl=false&sap-ui-density=sapUiSizeCompact

  • Select with UI5 Inspector pin button (ToggleButton, not the icon inside), press ToggleButton ---> press event is not logged
  • After that select one of the buttons in the "actions" aggregation of the header (for example, the pen button - the OverflowToolbarButton), press the pen button --> press events are logged, but for the Icon inside - _button0-img, although this is not selected in the ControlTree
  • Now select again the pin button (ToggleButton) and press the pen button --> press events are still being logged for _button0-img, although now a totally different button is selected.

I think there are some adjustments in the code needed, to prevent such issues in the behavior:

  1. this._lastSelectedControl should only be changed in 'do-control-select', where we are sure the selected control in the ControlsTree has been changed. It is not correct to set lastSelectedControl in .getEvents function, which purpose seems to be different and also it is called from different places multiple times.
  2. Before setting a new control as lastSelected, you need to remove all event listeners from the previously selected control, which dispatch the CustomEvent with action 'do-event-fired' /not sure why those listeners are attached at two places, especially in getEvents - I do not expect a getter function to do anything else, than returning some kind of data, without additional twists?/
  3. Only add new event listeners or remove the ones from the previously selected control, if the previously selected control is different than the currently selected one.

Additionally:

  1. 'do-event-fired' sends message with action 'on-control-select' - is this really needed? I guess you need it in order to update the Events view? In this case, can't you make another action (for example, 'on-event-fired') only passing controlEvents and you will only set the data for the control events in the handler? I think it should work this way, updating only the Events view?
  2. Why do you use the global window object for persistentFiredEventsMap, eventQueue and updateTimeout ID? I cannot see those properties to be used somewhere else outside the context of controlInformation. I think you should better use "this" and not pollute the global scope.

yanaminkova and others added 5 commits February 12, 2025 11:59

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@yanaminkova yanaminkova merged commit 80746de into master Mar 6, 2025
3 checks passed
@yanaminkova yanaminkova deleted the fired-events branch March 6, 2025 08:57
Copy link
Contributor

🎉 This PR is included in version 1.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants