Issue 6487 - IOElement as base component for ABP UI

Issue 6487 - IOElement as base component for ABP UI

[a.giammarchi, 2018-03-19 11:43:59]

Ready for review.

My only doubt is if we want to include the generated test file too.

[saroyanm, 2018-03-20 20:08:19]

Looks to be a great start, I only have couple of question, but in overal looks
good and I agree that no need to include generated test files.

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js
File js/io-element.js (right):

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js#ne...
js/io-element.js:20: static getID(element)
Question: Why is the generation of unique ID is required ?
(I assume it's a HyperHTMLElement related question)

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js#ne...
js/io-element.js:53: IOElement.intent("i18n", id =>
What does intent do ? I couldn't find it in the documentation, maybe I've
overlooked ?

[a.giammarchi, 2018-03-21 07:27:19]

I've answered Manvel, going to update the diff removing the test file.

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js
File js/io-element.js (right):

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js#ne...
js/io-element.js:20: static getID(element)
On 2018/03/20 20:08:19, saroyanm wrote:
> Question: Why is the generation of unique ID is required ?
> (I assume it's a HyperHTMLElement related question)

It's not, it's aria related question. Most of the aria helpers targets nodes by
ID. It's madness to find meaningful, unique, IDs per each `labelled-by` use case
and we already had discussions about the id-name when the only purpose of that
ID was to serve a11y.

Having a way to generate unique IDs when needed makes our life easier and
components content creation a no-brainer because you can always `<el
aria-labelled-by=${otherComponent.id}/>` and be sure there is one.

I hope I've answered your question.

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js#ne...
js/io-element.js:53: IOElement.intent("i18n", id =>
On 2018/03/20 20:08:19, saroyanm wrote:
> What does intent do ? I couldn't find it in the documentation, maybe I've
> overlooked ?

In HyperHTMLElement, being `define(...)` the method that actually defines the
class as Custom Element, the initial `hyperHTML.define(...)` is renamed
`.intent(...)`.

Documentation here: https://viperhtml.js.org/hyperhtml/documentation/#api-3

Please note that nobody has to use `intent`, unless really needed, because once
defined in here every parsing will consider interpolations with `{i18n:
'unique-id'}` objects as i18n content, effectively abandoning the class based
technique and putting text where it belongs: inside an element (at least
visually).

TL;DR this is a one off, that belongs to this root class only, to abstract away
i18n within every IOElement populated through hyperHTML.

[a.giammarchi, 2018-03-21 08:11:32]

Removed all test files (.gitignore) and manually removed tests/io-element.js in
mercurial due its inability to whitelist files.

Ready for review.

[a.giammarchi, 2018-03-21 08:15:57]

Explained some extra detail about the unique ID choice.

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js
File js/io-element.js (right):

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js#ne...
js/io-element.js:20: static getID(element)
I've forgotten an important detail. Once you have `<io-el>` with a unique id, it
is also easy/straight forward to recycle same ID internally while populating via
hyperHTML.

this.html`
  <input aria-labelled-by=${this.id + 'label'}>
  <label id=${this.id + 'label'}>hello</label>`;

This effectively make components both addressable and their own relevant parts
easy to retrieve too.

[saroyanm, 2018-03-21 11:22:22]

This is LGTM to start using hyperhtml, I still need to go through whole
documentation and start playing around with it, but happy that we are using it.

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js
File js/io-element.js (right):

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js#ne...
js/io-element.js:20: static getID(element)
On 2018/03/21 07:27:19, a.giammarchi wrote:
> On 2018/03/20 20:08:19, saroyanm wrote:
> > Question: Why is the generation of unique ID is required ?
> > (I assume it's a HyperHTMLElement related question)
> 
> It's not, it's aria related question. Most of the aria helpers targets nodes
by
> ID. It's madness to find meaningful, unique, IDs per each `labelled-by` use
case
> and we already had discussions about the id-name when the only purpose of that
> ID was to serve a11y.
> 
> Having a way to generate unique IDs when needed makes our life easier and
> components content creation a no-brainer because you can always `<el
> aria-labelled-by=${otherComponent.id}/>` and be sure there is one.
> 
> I hope I've answered your question.
We could also wrap element inside of the label tag if we would like to make it
labeled, but I agree that having better flexibility to reference the element is
a good approach.

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js#ne...
js/io-element.js:20: static getID(element)
On 2018/03/21 08:15:57, a.giammarchi wrote:
> I've forgotten an important detail. Once you have `<io-el>` with a unique id,
it
> is also easy/straight forward to recycle same ID internally while populating
via
> hyperHTML.
> 
> this.html`
>   <input aria-labelled-by=${this.id + 'label'}>
>   <label id=${this.id + 'label'}>hello</label>`;
> 
> This effectively make components both addressable and their own relevant parts
> easy to retrieve too.

Noted.

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js#ne...
js/io-element.js:53: IOElement.intent("i18n", id =>
On 2018/03/21 07:27:19, a.giammarchi wrote:
> On 2018/03/20 20:08:19, saroyanm wrote:
> > What does intent do ? I couldn't find it in the documentation, maybe I've
> > overlooked ?
> 
> In HyperHTMLElement, being `define(...)` the method that actually defines the
> class as Custom Element, the initial `hyperHTML.define(...)` is renamed
> `.intent(...)`.
Suggestion: Maybe we can document that somewhere, ex. in HyperHTMLElement README
?
> Documentation here: https://viperhtml.js.org/hyperhtml/documentation/#api-3
> 
> Please note that nobody has to use `intent`, unless really needed, because
once
> defined in here every parsing will consider interpolations with `{i18n:
> 'unique-id'}` objects as i18n content, effectively abandoning the class based
> technique and putting text where it belongs: inside an element (at least
> visually).
> 
> TL;DR this is a one off, that belongs to this root class only, to abstract
away
> i18n within every IOElement populated through hyperHTML.

Noted.

I still need to find time to go through the documentation.

https://codereview.adblockplus.org/29723623/diff/29729555/package.json
File package.json (right):

https://codereview.adblockplus.org/29723623/diff/29729555/package.json#newcode7
package.json:7: "bash4test:js": "eslint ./js/**/*.js && bash -c 'echo \"/*
eslint-disable */$(browserify --node $0)\">$1'",
Nit: we don't use usually numbers as abbr. renaming to "bashForTest" should be
fine for now IMO.

[a.giammarchi, 2018-03-21 14:21:32]

Updated HyperHTMLElement documentation + code (to point at such documentation).

Using a smoke folder for smoke testing UI.

Added some detail about smoke testing in the README.

Ready for (hopefully) last review.

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js
File js/io-element.js (right):

https://codereview.adblockplus.org/29723623/diff/29723624/js/io-element.js#ne...
js/io-element.js:53: IOElement.intent("i18n", id =>
On 2018/03/21 11:22:21, saroyanm wrote:
> On 2018/03/21 07:27:19, a.giammarchi wrote:
> > On 2018/03/20 20:08:19, saroyanm wrote:
> > > What does intent do ? I couldn't find it in the documentation, maybe I've
> > > overlooked ?
> > 
> > In HyperHTMLElement, being `define(...)` the method that actually defines
the
> > class as Custom Element, the initial `hyperHTML.define(...)` is renamed
> > `.intent(...)`.
> Suggestion: Maybe we can document that somewhere, ex. in HyperHTMLElement
README
> ?
> > Documentation here: https://viperhtml.js.org/hyperhtml/documentation/#api-3
> > 
> > Please note that nobody has to use `intent`, unless really needed, because
> once
> > defined in here every parsing will consider interpolations with `{i18n:
> > 'unique-id'}` objects as i18n content, effectively abandoning the class
based
> > technique and putting text where it belongs: inside an element (at least
> > visually).
> > 
> > TL;DR this is a one off, that belongs to this root class only, to abstract
> away
> > i18n within every IOElement populated through hyperHTML.
> 
> Noted.
> 
> I still need to find time to go through the documentation.

Done.

[saroyanm, 2018-03-21 16:11:35]

LGTM