-
Notifications
You must be signed in to change notification settings - Fork 159
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
Make custom events bubbling configurable #161
Comments
The change was made because it's the behavior I originally planned (it's the same behavior as bootstrap-datepicker's events), but forgot to implement just by accident. Until #157, I wrongly thought that I programmed to make the events bubble. So, I'm not willing to go back to the unintended state caused by the mistake. I think bubbling by default is more beneficial because it gives users flexibility on event handling, and they can also stop the bubbling just by calling For these reasons, even if I accept your suggestion and add the new option, I'll make it default to can-bubble. Now, I have a question. Which do you think more work is needed to make, the new event config option in the library or something like this in your program? import OriginalDatepicker from 'vanillajs-datepicker/Datepicker';
class Datepicker extend OriginalDatepicker {
constructor(element, options = {}, rangepicker = undefined) {
super(element, options, rangepicker);
['show', 'hide'].forEach((eventName) => {
element.addEventListener(eventName, (ev) => {
ev.stopPropagation();
});
});
};
} |
Thanks for the quick answer and your proposed fix. Yes, your proposed solution does indeed work (not sure why I did not think of it in the first place).
Thanks again |
For 1, I apologize for the inconvenience. Noticing my not only mistake (in such basic stuff) but also misunderstanding, I might have been kind of in a panic and rushed too much to recover the mistake. For 2, even if the events' constructor parameters are configurable via library's option, if the default is can-bubble, it seems pretty likely to me that most people just use stopPropagation() anyway because: I appreciate your offer, but I'm not interested in adding a feature that seems like most users don't care about. So, let's see if other people also want that option. |
For 1: I agree on the gray-zone feature argument. It was never documented either way, yet I think it's fair to assume that if something follows the default behaviour for an Event and has done so for various version that this was the developers intent, even if not documented. In any case, thanks for your time and the detailed responses. |
With the fix of #157 and PR #158 custom events are by default bubbling and cancelable.
In some settings -- such as mine -- this can lead to unwanted behavior, specifically in cases where bubbling should be suppressed.
Suggestion: Extend the Datepicker class and its options to make event bubbling and event cancellation configurable and use that information when calling triggerDatepickerEvent (undifferentiated by type for now). I also would suggest to set the default of both options to 'false' to recover the 1.3.3 version behavior (and also to follow the defined default in the Event interface).
Happy to support. Just wanted know if there is a particular reason for changing the default behavior and to clarify the (potential) way forward.
The text was updated successfully, but these errors were encountered: