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

Provide a way to prevent closing Webamp #655

Merged
merged 2 commits into from
Sep 20, 2018
Merged

Provide a way to prevent closing Webamp #655

merged 2 commits into from
Sep 20, 2018

Conversation

captbaritone
Copy link
Owner

@captbaritone captbaritone commented Sep 11, 2018

Fixes #653

Still need to add documentation and update index.d.ts.

@durasj
Copy link
Collaborator

durasj commented Sep 11, 2018

Cool, @captbaritone! Thinking about it now, if it'll be a separate callback which purpose is exactly this, wouldn't it be better to rather take the returned value? So false returned would mean the webamp should not be closed.

Adding the Event-like object there would make the callbacks a bit inconsistent.

@captbaritone
Copy link
Owner Author

captbaritone commented Sep 17, 2018

Hmm. I'm a bit skeptical of using a return value from the callback. Seems a little less explicit. Would somebody reading the code that returns false understand why it was doing that without a comment? Probably not.

Also, I don't see an obvious way to implement it.

I also agree that preventDefault is a bit confusing since it implies that we are preventing some kind of DOM event, which is... a bit of a stretch.

What if we passed the callback a "cancel" function which the user could conditionally call?

@captbaritone
Copy link
Owner Author

I've implemented the "cancel" approach here. Any thoughts @durasj ?

@durasj
Copy link
Collaborator

durasj commented Sep 20, 2018

Look better, @captbaritone! Sorry for the late reply. I think the biggest problem with supplying the event was that it could be then maybe expected also on other callbacks (for consistency).

The reason why I've proposed the false was that the onWillClose seemed to be documented as something specifically for this purpose. When the webamp is like "hey, I will close myself" and callback can be "No! Stop! False!" :).

But cancel makes sense, the user just needs to keep in mind it can't be called asynchronously.

@captbaritone
Copy link
Owner Author

Hmm. Good point. Passing a callback does leave open the possibility that a user might think they could call it async. I hadn’t thought of that! I think I still like it better though, so i’ll Go ahead and ship this.

Thanks again for the typo fix!!

@captbaritone captbaritone merged commit ff06c96 into master Sep 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants