Skip to content

* added the html5 Notification api #146

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

Closed
wants to merge 2 commits into from
Closed

* added the html5 Notification api #146

wants to merge 2 commits into from

Conversation

coreyauger
Copy link
Contributor

Added Notification API as outlined here (https://developer.mozilla.org/en-US/docs/Web/API/notification)

@coreyauger
Copy link
Contributor Author

I was thinking that it would be nice to beak out the notification options into an options trait .. rather then passing it in as a js.Object like I am doing now. Thoughts?

@mseddon
Copy link
Contributor

mseddon commented Sep 1, 2015

Yep, I'd definitely do that if you can, with a companion object to construct it, see https://github.com/scala-js/scala-js-dom/blob/master/src/main/scala/org/scalajs/dom/raw/lib.scala#L3824 or https://github.com/scala-js/scala-js-dom/blob/master/src/main/scala/org/scalajs/dom/raw/lib.scala#L4293.

It looks like this api is not supported under IE or mobile browsers, so it may also better live in the experimental package. We use implicit conversions defined in there to overlay support for apis that won't "just work" everywhere, to avoid the confusion of accidentally relying on something our preferred browsers happen to support, see: https://github.com/scala-js/scala-js-dom/blob/master/src/main/scala/org/scalajs/dom/experimental/Vibration.scala for an example.

@mseddon
Copy link
Contributor

mseddon commented Sep 1, 2015

I would also change the name of the commit to fall in line with the project conventions. It should read `Add the Notification API." - present tense, no leading *. (and "html5" is possibly redundant here).

One last thing- I notice some of your lines are quite long. A lot of the contributors review PRs on mobile, so try to keep the lines to 80 characters if possible, and there are a few places with consecutive blank lines- they should just be single. See: https://github.com/scala-js/scala-js/blob/master/CODINGSTYLE.md

@coreyauger
Copy link
Contributor Author

Sounds good... I am going to close this and issue a new pull request. Thanks.

@coreyauger coreyauger closed this Sep 1, 2015
@mseddon mseddon mentioned this pull request Sep 1, 2015
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