Re: [w3c/manifest] Feat(events): add BeforeInstallPromptEvent (#520)

mgiuca commented on this pull request.

Looks pretty good. I have some quick comments, but I haven't had a deeper look at it and I want to do that tomorrow (on my workstation where I've got the repo checked out so I can build it and look at it in HTML). Can we wait until then to land this?

> @@ -447,6 +447,170 @@ <h3 id="installation-sec">
         DOM events <a>fired</a> by this specification use the <dfn>application
         life-cycle task source</dfn>.
       </p>
+      <section data-dfn-for="BeforeInstallPromptEvent">
+        <h3>
+          <code>BeforeInstallPromptEvent</code> Interface
+        </h3>
+        <pre class="idl">
+          [Constructor(DOMString typeArg, optional BeforeInstallPromptEventInit eventInit)]
+          interface BeforeInstallPromptEvent : Event {
+              Promise&lt;UserResponseObject&gt; prompt();
+          };
+
+          dictionary BeforeInstallPromptEventInit : EventInit {
+            AppBannerPromptOutcome userChoice;
+          };
+
+          dictionary UserResponseObject {

Why not PromptResponseObject?

> +        <p>
+          The <dfn>BeforeInstallPromptEvent</dfn> is dispatched prior to
+          activating an <a>automated install prompt</a>, allowing a developer
+          to prevent the default action for an install prompt.
+        </p>
+        <p>
+          Thus, the default action of the <a>BeforeInstallPromptEvent</a> is to
+          <a data-lt="presents an install prompt">present an automated install
+          prompt</a> to the end-user. Canceling the default action (via
+          <code>.preventDefault()</code>) prevents the user agent from
+          <a data-lt="presents an install prompt">presenting an automated
+          install prompt</a> until a later time (see
+          <a>BeforeInstallPromptEvent.prompt</a>() method).
+        </p>
+        <p>
+          The <a>BeforeInstallPromptEvent</a> has three internal slots, which

Drop "three"; that sort of language can get out of date without anyone noticing.

> @@ -447,6 +447,170 @@ <h3 id="installation-sec">
         DOM events <a>fired</a> by this specification use the <dfn>application
         life-cycle task source</dfn>.
       </p>
+      <section data-dfn-for="BeforeInstallPromptEvent">
+        <h3>
+          <code>BeforeInstallPromptEvent</code> Interface
+        </h3>
+        <pre class="idl">
+          [Constructor(DOMString typeArg, optional BeforeInstallPromptEventInit eventInit)]
+          interface BeforeInstallPromptEvent : Event {
+              Promise&lt;UserResponseObject&gt; prompt();
+          };
+
+          dictionary BeforeInstallPromptEventInit : EventInit {
+            AppBannerPromptOutcome userChoice;

I'm still not a fan of allowing this as an argument (I don't see why this is ever useful and it's weird that it's an outcome and not a promise). I don't remember the outcome of the discussion last time I brought it up. I want to have a closer look tomorrow.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/manifest/pull/520#pullrequestreview-7570834