Issue 5542: Implement tooltips for new options page

Already landed / irrelevant at this point.

[a.giammarchi, 2018-02-05 12:37:22]

ehr .. sorry for the mess with changes, I was pushing with the wrong rev.

I hope this version meets criteria, improves a11y, and works fine.

Please review when you have time, thank you.

[sergei, 2018-02-19 10:18:42]

I just stumbled upon it accidentally (didn't even check the code) and wanted to
ask a couple of questions, please don't consider me as a reviewer.

I thought that to be consistent with other commits the title should be in the
following format
Issue 5542 - Implement tooltips for new options page
instead of current
Issue #5542: Implement tooltips for new options page

https://codereview.adblockplus.org/29683678/diff/29689598/desktop-options.js
File desktop-options.js (right):

https://codereview.adblockplus.org/29683678/diff/29689598/desktop-options.js#...
desktop-options.js:1404: const anchors =
document.querySelectorAll("[data-tooltip]");
I thought we don't use `const` in such cases, do we?

[a.giammarchi, 2018-02-19 10:26:39]

https://codereview.adblockplus.org/29683678/diff/29689598/desktop-options.js
File desktop-options.js (right):

https://codereview.adblockplus.org/29683678/diff/29689598/desktop-options.js#...
desktop-options.js:1404: const anchors =
document.querySelectorAll("[data-tooltip]");
On 2018/02/19 10:18:42, sergei wrote:
> I thought we don't use `const` in such cases, do we?

why not, if I might ask? `const` is used to guard against undesired
reassignments. It's true that in this file I actually should drop all `let` and
use `const` instead, but for new code, where it makes sense, I don't see why we
shouldn't use language guards/features.

If this was explicitly written in some code guideline thought, I'd be happy to
stick with it and preserve consistency. Yet I don't understand why would we make
such decision.

Thanks in advance for any sort of clarification.

[sergei, 2018-02-19 10:44:45]

https://codereview.adblockplus.org/29683678/diff/29689598/desktop-options.js
File desktop-options.js (right):

https://codereview.adblockplus.org/29683678/diff/29689598/desktop-options.js#...
desktop-options.js:1404: const anchors =
document.querySelectorAll("[data-tooltip]");
On 2018/02/19 10:26:38, a.giammarchi wrote:
> On 2018/02/19 10:18:42, sergei wrote:
> > I thought we don't use `const` in such cases, do we?
> 
> why not, if I might ask? `const` is used to guard against undesired
> reassignments.
Yep, I know what it is for but I got several times in codereviwes that we should
use `let` in such cases. That's because I'm asking to know the current state.
> It's true that in this file I actually should drop all `let` and
> use `const` instead, but for new code, where it makes sense, I don't see why
we
> shouldn't use language guards/features.
> 
> If this was explicitly written in some code guideline thought, I'd be happy to
> stick with it and preserve consistency. Yet I don't understand why would we
make
> such decision.
I find the wording actually vague
https://adblockplus.org/en/coding-style#javascript
"> Use const for constant expressions that could as well have been inlined (e.g.
static parameters, imported modules, etc.)."
> 
> Thanks in advance for any sort of clarification.

[a.giammarchi, 2018-02-19 10:53:06]

https://codereview.adblockplus.org/29683678/diff/29689598/desktop-options.js
File desktop-options.js (right):

https://codereview.adblockplus.org/29683678/diff/29689598/desktop-options.js#...
desktop-options.js:1404: const anchors =
document.querySelectorAll("[data-tooltip]");
On 2018/02/19 10:44:45, sergei wrote:
> I find the wording actually vague
> https://adblockplus.org/en/coding-style#javascript
> "> Use const for constant expressions that could as well have been inlined
(e.g.
> static parameters, imported modules, etc.)."
> > 

that's the second point though, the first one says:

> Always use block-scoping (let / const), except when sharing global variables
between scripts cannot be avoided.

The for/of is block-scoping so I think I've followed the rules.