sebfz1 / wicket-jquery-ui

jQuery UI & Kendo UI integration in Wicket
http://www.7thweb.net/wicket-jquery-ui/
Other
93 stars 58 forks source link

No way to control DialogBehavior behaviours in AbstractDialog. #291

Closed Jezza closed 6 years ago

Jezza commented 6 years ago

The newWidgetBehavior method is final, as such, we can't replace or customise the return value for newOnDefaultCloseAjaxBehavior. That locks out a lot of possible customisation options. I understand why you made it final, so you could handle various parts of the form, etc in the AbstractFormDialog, but I don't think it's much of a concern. If someone overrides newWidgetBehaviour they either know what they're doing, or it's just going to stop working. I'd prefer something like wicket's style. It has public internal API methods that you shouldn't use unless you know what you're doing. I'd make it as a that, and make it non-final.

Also, make CloseEvent public as well, otherwise we can't access it.

I'll submit a PR if you have no issue with this.

sebfz1 commented 6 years ago

Hi, I'm on vacation for a few days, I will look at this next week ! :)

sebfz1 commented 6 years ago

I've removed all final modifier on all newWidgetBehavior method, and created needed factory methods for XxxListenerWrapper (wicket8.x branch).

About CloseEvent, you can access it when you override DialogBehavior. If you access it outside this scope, I'd like to understand why before changing. My point is that all XxxEvent extends JQueryEvent are protected in the API (no exception). If I change for one, I'd rather like to change for all for consistency.

Jezza commented 6 years ago

Sweet, the changes look good.

About the CloseEvent, I think the main reason I wanted it public was because we were posting another CloseEvent to the dialog, so panels and whatnot could cleanup after a dialog close, if they supported it.

And I think it was just a matter of unifying the events, but after some thought, yeah, the JQueryEvents are mostly internal use, so I'm fine with you leaving it the way it is.