Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1215)

Issue 29683678: Issue 5542: Implement tooltips for new options page (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by a.giammarchi
Modified:
8 months, 2 weeks ago
Reviewers:
CC:
saroyanm, Thomas Greiner, sergei
Visibility:
Public.

Description

Already landed / irrelevant at this point.

Patch Set 1 #

Patch Set 2 : made tooltip a11y friendly/focusable #

Patch Set 3 : #

Patch Set 4 : cleaning up the patch #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+36 lines, -18 lines) Patch
M desktop-options.html View 1 2 chunks +4 lines, -4 lines 0 comments Download
M desktop-options.js View 1 2 3 2 chunks +25 lines, -13 lines 4 comments Download
M skin/desktop-options.css View 1 2 2 chunks +7 lines, -1 line 0 comments Download

Messages

Total messages: 6
a.giammarchi
1 year, 5 months ago (2018-01-29 16:22:46 UTC) #1
a.giammarchi
ehr .. sorry for the mess with changes, I was pushing with the wrong rev. ...
1 year, 5 months ago (2018-02-05 12:37:22 UTC) #2
sergei
I just stumbled upon it accidentally (didn't even check the code) and wanted to ask ...
1 year, 5 months ago (2018-02-19 10:18:42 UTC) #3
a.giammarchi
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#newcode1404 desktop-options.js:1404: const anchors = document.querySelectorAll("[data-tooltip]"); On 2018/02/19 10:18:42, sergei wrote: ...
1 year, 5 months ago (2018-02-19 10:26:39 UTC) #4
sergei
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#newcode1404 desktop-options.js:1404: const anchors = document.querySelectorAll("[data-tooltip]"); On 2018/02/19 10:26:38, a.giammarchi wrote: ...
1 year, 5 months ago (2018-02-19 10:44:45 UTC) #5
a.giammarchi
1 year, 5 months ago (2018-02-19 10:53:06 UTC) #6
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5