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

Issue 29770568: Issue 6645 - Collapse elements through user style sheets if possible (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 2 months ago by Sebastian Noack
Modified:
1 year, 2 months ago
Reviewers:
Manish Jethani
CC:
Oleksandr, kzar
Visibility:
Public.

Description

Issue 6645 - Collapse elements through user style sheets if possible

Patch Set 1 #

Patch Set 2 : Handle missing CSS.escape() on Microsoft Edge #

Patch Set 3 : Cleaned up code #

Total comments: 7

Patch Set 4 : Addressed Manish's feedback #

Total comments: 5

Patch Set 5 : Rebased #

Patch Set 6 : Got rif of hasSrc variable #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -36 lines) Patch
M include.preload.js View 1 2 3 4 5 3 chunks +43 lines, -36 lines 0 comments Download

Messages

Total messages: 8
Sebastian Noack
1 year, 2 months ago (2018-05-04 14:22:15 UTC) #1
Manish Jethani
https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js#newcode138 include.preload.js:138: // the "visisiblity" CSS property. Typo: visibility https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js#newcode155 include.preload.js:155: ...
1 year, 2 months ago (2018-05-07 17:42:02 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js#newcode138 include.preload.js:138: // the "visisiblity" CSS property. On 2018/05/07 17:42:02, Manish ...
1 year, 2 months ago (2018-05-09 16:30:46 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js#newcode166 include.preload.js:166: if (value && attr in element) On 2018/05/09 16:30:46, ...
1 year, 2 months ago (2018-05-09 23:11:09 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js#newcode163 include.preload.js:163: for (let attr of ["src", "srcset"]) On 2018/05/09 23:11:09, ...
1 year, 2 months ago (2018-05-10 01:08:23 UTC) #5
Manish Jethani
https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js#newcode162 include.preload.js:162: let hasSrc = false; Only one small nit, and ...
1 year, 2 months ago (2018-05-10 01:57:17 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js#newcode162 include.preload.js:162: let hasSrc = false; On 2018/05/10 01:57:17, Manish Jethani ...
1 year, 2 months ago (2018-05-10 11:45:34 UTC) #7
Manish Jethani
1 year, 2 months ago (2018-05-11 04:08:52 UTC) #8
On 2018/05/10 11:45:34, Sebastian Noack wrote:
> https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js
> File include.preload.js (right):
> 
>
https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js#...
> include.preload.js:162: let hasSrc = false;
> On 2018/05/10 01:57:17, Manish Jethani wrote:
> > Only one small nit, and this is totally a nit. We could avoid the hasSrc
> > variable here by setting selector to "" initially, then later:
> > 
> >   if (selector)
> >     return element.localName + selector;
> > 
> >   return null;
> 
> Done.

LGTM
Sign in to reply to this message.

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