Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29673576: Make new options page compatible with edge (Closed)

Created:
Jan. 18, 2018, 11:13 a.m. by a.giammarchi
Modified:
Feb. 8, 2018, 3:38 p.m.
Visibility:
Public.

Description

Directly from ticket https://issues.adblockplus.org/ticket/5813, I have changed few CSS things unavailable in Edge and took the opportunity to use native closest API where appropriate and double checked on VirtualBox Win10 + Edge that everything looks the same. I am not 100% sure how to counter-verify that everything also works well but by clicking around "all the clickable" I think it's fine.

Patch Set 1 #

Total comments: 18

Patch Set 2 : applied changes #

Total comments: 3

Patch Set 3 : #

Patch Set 4 : dropped automatically inserted extra line #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+119 lines, -45 lines) Patch
M desktop-options.js View 1 3 chunks +10 lines, -16 lines 6 comments Download
M skin/desktop-options.css View 1 2 3 11 chunks +109 lines, -29 lines 0 comments Download

Messages

Total messages: 14
a.giammarchi
Please let me know if I forgot anything or if there is something else to ...
Jan. 18, 2018, 11:46 a.m. (2018-01-18 11:46:19 UTC) #1
Thomas Greiner
Any improvement helps and in the end we'd need to involve QA in it to ...
Jan. 18, 2018, 4:20 p.m. (2018-01-18 16:20:47 UTC) #2
a.giammarchi
just replying to Thomas points https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#newcode539 desktop-options.js:539: if (returnElement) return element; ...
Jan. 19, 2018, 11:16 a.m. (2018-01-19 11:16:48 UTC) #3
Thomas Greiner
https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#newcode539 desktop-options.js:539: if (returnElement) return element; On 2018/01/19 11:16:48, a.giammarchi wrote: ...
Jan. 19, 2018, 1:55 p.m. (2018-01-19 13:55:16 UTC) #4
a.giammarchi
answering https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#newcode539 desktop-options.js:539: if (returnElement) return element; On 2018/01/19 13:55:16, Thomas ...
Jan. 19, 2018, 2:16 p.m. (2018-01-19 14:16:28 UTC) #5
a.giammarchi
Updated as requested. I have left `el.closest(...)` as it is more simple due my current ...
Jan. 19, 2018, 5:02 p.m. (2018-01-19 17:02:29 UTC) #6
Thomas Greiner
Just a typo and comments on seemingly unintentional changes. https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29673577/desktop-options.js#newcode539 desktop-options.js:539: ...
Jan. 22, 2018, 1:06 p.m. (2018-01-22 13:06:38 UTC) #7
a.giammarchi
fixed all the things! It'd be awesome if we could move this forward, thanks.
Feb. 5, 2018, 9:47 a.m. (2018-02-05 09:47:05 UTC) #8
Thomas Greiner
LGTM
Feb. 5, 2018, 4:56 p.m. (2018-02-05 16:56:57 UTC) #9
saroyanm
Sorry folks this went out of my radar as I wasn't in the CC. https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js ...
Feb. 8, 2018, 2:59 p.m. (2018-02-08 14:59:09 UTC) #10
a.giammarchi
https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js#newcode536 desktop-options.js:536: element = element.closest(`[data-${dataName}]`); On 2018/02/08 14:59:09, saroyanm wrote: > ...
Feb. 8, 2018, 3:07 p.m. (2018-02-08 15:07:21 UTC) #11
saroyanm
https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js#newcode536 desktop-options.js:536: element = element.closest(`[data-${dataName}]`); On 2018/02/08 15:07:21, a.giammarchi wrote: > ...
Feb. 8, 2018, 3:20 p.m. (2018-02-08 15:20:18 UTC) #12
a.giammarchi
https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js#newcode536 desktop-options.js:536: element = element.closest(`[data-${dataName}]`); On 2018/02/08 15:20:18, saroyanm wrote: > ...
Feb. 8, 2018, 3:27 p.m. (2018-02-08 15:27:29 UTC) #13
saroyanm
Feb. 8, 2018, 3:38 p.m. (2018-02-08 15:38:30 UTC) #14
Message was sent while issue was closed.
https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js
File desktop-options.js (right):

https://codereview.adblockplus.org/29673576/diff/29689576/desktop-options.js#...
desktop-options.js:536: element = element.closest(`[data-${dataName}]`);
On 2018/02/08 15:27:29, a.giammarchi wrote:
> On 2018/02/08 15:20:18, saroyanm wrote:
> > On 2018/02/08 15:07:21, a.giammarchi wrote:
> > > On 2018/02/08 14:59:09, saroyanm wrote:
> > > > Detail: We are usually careful when it comes to experimental
technologies.
> > > > 
> > > > Make sense to find out which Edge versions are we supporting.
> > > > This feature is only supported in edge 15 and up.
> > > > 
> > > > Ollie, can you please let us know which versions of Edge are we
supporting
> ?
> > > > 
> > > > Probably make sense to update that info in as well:
> > > > https://adblockplus.org/en/requirements
> > > 
> > > FWIW closest is not really experimental, it has been around for years, but
> in
> > 
> > According to MDN it is
> 
> don't trust everything you read in MDN ... it's updated by devs that very
often
> forget to update.
> 
> closest is in the living standard since years now, it's been polyfilled 3+
years
> ago -> https://github.com/WebReflection/dom4 and IE15 was last one to
implement
> it.
> 
> It was a good catch/safety call but also both me and I guess Thomas too
checked
> before shipping it.
> 
> I'd like to share this little CLI that is configured with our target browsers
> and tell instantly if we can support this or that. Let's follow up in irc.

Thanks for the links and info.
Acknowledged.

Powered by Google App Engine
This is Rietveld