OpenFin Notifications #7

BenLambertNcl · 2017-03-10T12:20:12Z

Basic implementation of the OpenFin notifications.

The OpenFin API requires a html template to use as its notification. I have included a very simple html template that resembles the default electron notification. OpenFin does not support native notifications.

OpenFin does not allow preload scripts like electron. Because of this, npm posinstall copies the API into the src directory, which is accessed via a script tag. I also included a blank index.js file in the specification project to avoid any 404 errors from electron/browsers.

ColinEberhardt · 2017-03-10T14:07:00Z

packages/api-openfin/README.md

+npm install ssf-desktop-api-openfin --save
+```
+
+You need to include this code into the project by loading the javascript file directly (as Openfin does not support preloading).


JavaScript
OpenFin

Yes, I really am that pedantic!

Beat me to it! 😄

ColinEberhardt

Apart from a very pedantic comment about capitalisation, looks good to me 👍

jleft

A couple of questions and some very minor comments, but overall looking really good to me!

jleft · 2017-03-10T12:55:56Z

packages/api-openfin-demo/package.json

+ "author": "",
+ "license": "Apache-2.0",
+ "dependencies": {
+   "bootstrap": "^3.3.7",


I don't think this is used - remove?

jleft · 2017-03-10T12:56:02Z

packages/api-openfin-demo/package.json

+ "license": "Apache-2.0",
+ "dependencies": {
+   "bootstrap": "^3.3.7",
+   "jquery": "^3.1.1",


I don't think this is used - remove?

jleft · 2017-03-10T12:56:59Z

packages/api-openfin-demo/app.json

+        "description": "",
+        "name": "SSF API Demo Openfin"
+    }
+}


Nitpick: add newline at the end of the file

jleft · 2017-03-10T13:02:39Z

packages/api-openfin/README.md

@@ -0,0 +1,13 @@
+# Symphony API for Openfin


Openfin => OpenFin

jleft · 2017-03-10T13:02:44Z

packages/api-openfin/README.md

@@ -0,0 +1,13 @@
+# Symphony API for Openfin
+
+This project provides an implementation of the Symphony Desktop Wrapper API Specification for Openfin


Openfin => OpenFin

jleft · 2017-03-10T13:08:45Z

packages/api-openfin-demo/README.md

@@ -0,0 +1,11 @@
+# Symphony API for Openfin Demo


Openfin has the f capitalised. Please can Openfin be changed to OpenFin?

jleft · 2017-03-10T13:08:47Z

packages/api-openfin-demo/README.md

@@ -0,0 +1,11 @@
+# Symphony API for Openfin Demo
+
+This project provides a demonstration of the Symphony Desktop Wrapper API within Openfin.


Openfin => OpenFin

jleft · 2017-03-10T13:11:28Z

packages/api-openfin/index.js

@@ -0,0 +1,13 @@
+/* globals fin  */


I think this could be required in multiple files in the future - can this be added to a .eslintrc.json file at the root of this package, packages/api-openfin/?

{ "globals": { "fin": true } }

jleft · 2017-03-10T13:34:18Z

packages/api-openfin/index.js

+window.Notification = function(title, options) {
+  const message = {
+    title: title,
+    options: options


Will these options do anything?

I.e. if I look in our API specification, the options include dir, lang etc. If they don't do anything then I think we can omit them from the object passed to OpenFin and only pass the message, but keep options in our API and we can add support for dir, lang` etc. later.

jleft · 2017-03-10T14:08:38Z

packages/api-openfin/notification.html

@@ -0,0 +1,11 @@
+<h5 id="message">Message</h5>


Assuming that this page needs to be valid HTML, worth updating to something like this?

<!DOCTYPE html> <meta charset="utf-8"> <title></title> <style> html { border: solid 2px black; } </style> <h5 id="message"></h5> <script> function onNotificationMessage(message) { document.getElementById('message').innerText = message.title; } </script>

Might be worth keeping the h5 empty too, without the text Message?

<h5 id="message"></h5>

OpenFin notifications are html pages in their own right, and this is just injected inside the body of that page. I can still change it to be like this (but without the <!DOCTYPE html>) if you prefer?

OpenFin notifications are html pages in their own right, and this is just injected inside the body of that page.

Are you sure that this is just injected into the body? From the docs and examples it looks like it's the entire HTML page.

This is the output from the notification box, with extra tags for html, body and head that were not included in the template (This is with the changes you suggested, bar the doctype).

AFAIK that's just the way Chrome handles minimal HTML5 documents. It adds in the html, head, body elements in the correct locations.

Ah, I see - when using dev tools, you're not seeing the original HTML source of the page which has been loaded - it's showing the DOM, the browsers interpretation of the HTML.

The browser will try to add missing elements, correct invalid markup etc. Therefore the original HTML can differ from the DOM shown in the dev tools. I don't OpenFin is injecting the template into the body here, I think it's just the browser.

It's still very minor, as the browser can correct it, but I think it's worth ensuring the HTML is valid, with doctype et. al.

That makes sense. 👍

BenLambertNcl added 2 commits March 10, 2017 12:18

Add Openfin notification API

505718d

Add Openfin API demo

0c4680c

BenLambertNcl requested a review from jleft March 10, 2017 12:20

Add readme for API and demo

d42fc9c

ColinEberhardt reviewed Mar 10, 2017

View reviewed changes

ColinEberhardt approved these changes Mar 10, 2017

View reviewed changes

jleft suggested changes Mar 10, 2017

View reviewed changes

BenLambertNcl changed the title ~~Openfin notifications~~ OpenFin Notifications Mar 10, 2017

BenLambertNcl force-pushed the Openfin-Notifications branch 2 times, most recently from 82940cc to 94db671 Compare March 10, 2017 15:12

Fix naming of OpenFin and JavaScript, Reorganise notification template

f4db645

BenLambertNcl force-pushed the Openfin-Notifications branch from 94db671 to f4db645 Compare March 10, 2017 15:13

jleft approved these changes Mar 10, 2017

View reviewed changes

jleft merged commit 6f38156 into symphonyoss:master Mar 10, 2017

BenLambertNcl deleted the Openfin-Notifications branch March 10, 2017 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenFin Notifications #7

OpenFin Notifications #7

BenLambertNcl commented Mar 10, 2017 •

edited

ColinEberhardt Mar 10, 2017

jleft Mar 10, 2017

ColinEberhardt left a comment

jleft left a comment

jleft Mar 10, 2017

jleft Mar 10, 2017

jleft Mar 10, 2017

jleft Mar 10, 2017

jleft Mar 10, 2017

jleft Mar 10, 2017

jleft Mar 10, 2017

jleft Mar 10, 2017

jleft Mar 10, 2017

jleft Mar 10, 2017

BenLambertNcl Mar 10, 2017

jleft Mar 10, 2017

BenLambertNcl Mar 10, 2017 •

edited

ColinEberhardt Mar 10, 2017

jleft Mar 10, 2017

BenLambertNcl Mar 10, 2017

		@@ -0,0 +1,13 @@
		# Symphony API for Openfin

		This project provides an implementation of the Symphony Desktop Wrapper API Specification for Openfin

		@@ -0,0 +1,11 @@
		# Symphony API for Openfin Demo

		This project provides a demonstration of the Symphony Desktop Wrapper API within Openfin.

OpenFin Notifications #7

OpenFin Notifications #7

Conversation

BenLambertNcl commented Mar 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColinEberhardt left a comment

Choose a reason for hiding this comment

jleft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenLambertNcl Mar 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenLambertNcl commented Mar 10, 2017 •

edited

BenLambertNcl Mar 10, 2017 •

edited