Issue 6514 - IOToggle Custom Element

js/io-toggle.js

-/* globals module, require */

[Thomas Greiner, 2018/04/25 17:29:14]

I noticed that none of the new files in this review contain our license header
so let's add that.

[Thomas Greiner, 2018/04/25 17:29:14]

Detail: You don't use `module` in this module.

[a.giammarchi, 2018/04/26 10:44:52]

To be honest, I feel like we should have module and require as always allowed in
our eslint configuration since we decided to use CommonJS for the time being.

[a.giammarchi, 2018/04/26 10:44:52]

Will do.

[Thomas Greiner, 2018/04/26 17:00:52]

Wouldn't .eslintrc.json be a more suitable place to put that then?

[a.giammarchi, 2018/04/27 10:59:04]

absolutely!

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-present eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
 
 "use strict";
 
 const IOElement = require("./io-element");
+const {boolean} = IOElement.utils;
 
 class IOToggle extends IOElement
 {
   // action, checked, and disabled should be reflected down the button
   static get observedAttributes()
   {
     return ["action", "checked", "disabled"];
   }
 
   created()
   {
     this.addEventListener("click", this);
     this.render();
   }
 
   get checked()
   {
     return this.hasAttribute("checked");
   }
 
   set checked(value)
   {
-    booleanAttribute.call(this, "checked", value);
+    boolean.attribute(this, "checked", value);
     this.render();
   }
 
   get disabled()
   {
     return this.hasAttribute("disabled");
   }
 
   set disabled(value)
   {
-    booleanAttribute.call(this, "disabled", value);
-    this.firstElementChild.disabled = this._disabled;

[Thomas Greiner, 2018/04/25 17:29:14]

Why don't you call `this.render()` here?

[a.giammarchi, 2018/04/26 10:44:52]

it's not needed, the disabled is the native DOM accessor and the render wouldn't
care less.

Just as extra info, hyperHTML does not create new DOM per each render, it simply
updates what it's responsible for through nodes that are always the same.

You can references these nodes even after rendering them, it does not lock you
in.

[Thomas Greiner, 2018/04/26 17:00:52]

I mean instead of setting `this.firstElementChild.disabled = this._disabled`
setting the button's disabled attribute in `render()`. That way we wouldn't need
to worry about what the component's first child element is. Also, all of the
rendering logic would be kept in a single location.

[a.giammarchi, 2018/04/27 10:59:04]

I'll try that. The issue here is that disabled is an accessor. Setting it as
attribute would not produce the accessor effect, will just set such attribute.

However, what you says makes sense so let's see how that works.

[Thomas Greiner, 2018/05/02 11:53:28]

Thanks, looking good!

+    boolean.attribute(this, "disabled", value);
+    this.render();
   }
 
   onclick(event)
   {
     if (!this.disabled)
     {
       this.checked = !this.checked;
-      if (this.ownerDocument.activeElement !== this.firstElementChild)
+      if (this.ownerDocument.activeElement !== this.child)
       {
-        this.firstElementChild.focus();
+        this.child.focus();
       }
       this.dispatchEvent(new CustomEvent("change", {
         bubbles: true,
         cancelable: true,
         detail: this.checked
       }));
     }
   }
 
   render()
   {
     this.html`
     <button
       role="checkbox"
+      disabled="${this.disabled}"
       data-action="${this.action}"
       aria-checked="${this.checked}"
       aria-disabled="${this.disabled}"
     />`;
   }
 }
 
 IOToggle.define("io-toggle");
-
-function asBoolean(value)

[Thomas Greiner, 2018/04/25 17:29:14]

Suggestion: "asBoolean" is quite close to "isBoolean" so it may cause issues.
Therefore what about using a more distinct name such as "toBoolean"?

[a.giammarchi, 2018/04/26 10:44:51]

will do

-{
-  return typeof value === "string" ? JSON.parse(value) : !!value;

[Thomas Greiner, 2018/04/25 17:29:14]

It's a bit unexpected that this function accepts everything except for strings
which contain invalid JSON. So is it intentional that you throw an error here
when a string with invalid JSON is passed?

[a.giammarchi, 2018/04/26 10:44:52]

only strings can be JSON, and in this case we are talking about string `true`
and string `false` as attributes value.

also string 1 and string 0 will be OK. Any other value should never be allowed
so I would expect an error.

[Thomas Greiner, 2018/04/26 17:00:52]

I can follow your line or argument right until the last part because why would
`"123"` throw if `123` doesn't?

So instead, what about the following so that we treat values the same way?

if (typeof value === "string")
{
  try
  {
    value = JSON.parse(value);
  }
  catch (ex)
  {
    // Ignore invalid JSON to continue using value as string
  }
}

return !!value;

[a.giammarchi, 2018/04/27 10:59:04]

will do.

-}
-
-function booleanAttribute(name, value)

[Thomas Greiner, 2018/04/25 17:29:14]

Detail: This function looks useful also for other components so what about
moving it to a shared location?

[a.giammarchi, 2018/04/26 10:44:51]

Usually the way I go is: as soon as more than a component needs it, I'll create
that file.

Right now I don't remember if any other components needs something like this,
but if this could be the starting point to create shared DOM utils then let's do
it.

Where do you think such utility should go?

[Thomas Greiner, 2018/04/26 17:00:52]

That's a fair point. And it should be fine as long as we don't duplicate
functions across multiple files because we forgot that we already have them
somewhere.
Not sure, maybe js/io-element.js?! At least it could be useful for any custom
element we'll introduce.

[a.giammarchi, 2018/04/27 10:59:05]

makes sense.

-{
-  if (asBoolean(value))
-  {
-    this.setAttribute(name, "true");
-  }
-  else
-  {
-    this.removeAttribute(name);
-  }
-}