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

Issue 29607560: Issue 6003 - Better handling of no-content-for-platform-message on help.eyeo.com (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by ire
Modified:
1 day, 1 hour ago
Reviewers:
juliandoucette
Base URL:
https://hg.adblockplus.org/help.eyeo.com
Visibility:
Public.

Description

Issue 6003 - Better handling of no-content-for-platform-message on help.eyeo.com

Patch Set 1 #

Total comments: 3

Patch Set 2 : Rebase #

Total comments: 12

Patch Set 3 : Hide only with hidden attribute, change btn to button #

Patch Set 4 : Update no content for platform message #

Total comments: 11

Patch Set 5 : Use data attributes instead of classname #

Total comments: 20

Patch Set 6 : Rebased #

Patch Set 7 : Addressed NITs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+56 lines, -29 lines) Patch
M static/src/js/main.js View 1 2 3 4 5 6 3 chunks +34 lines, -22 lines 0 comments Download
M static/src/scss/components/_article.scss View 1 2 3 4 5 6 1 chunk +4 lines, -3 lines 0 comments Download
M static/src/scss/content/_typography.scss View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download
M templates/article.tmpl View 1 2 3 4 5 6 1 chunk +13 lines, -4 lines 0 comments Download

Messages

Total messages: 16
ire
1 month ago (2017-11-14 12:00:13 UTC) #1
ire
Ready for review https://codereview.adblockplus.org/29607560/diff/29607561/static/scss/components/_article.scss File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29607561/static/scss/components/_article.scss#newcode33 static/scss/components/_article.scss:33: #no-content-for-platform-message I thought it looked better ...
1 month ago (2017-11-14 12:02:35 UTC) #2
juliandoucette
https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/components/_article.scss File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/components/_article.scss#newcode33 static/scss/components/_article.scss:33: #no-content-for-platform-message NIT: Why should this article content have a ...
4 weeks, 1 day ago (2017-11-15 13:27:45 UTC) #3
ire
https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/components/_article.scss File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/components/_article.scss#newcode33 static/scss/components/_article.scss:33: #no-content-for-platform-message On 2017/11/15 13:27:44, juliandoucette wrote: > NIT: Why ...
4 weeks ago (2017-11-15 16:57:31 UTC) #4
juliandoucette
https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/components/_article.scss File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/components/_article.scss#newcode33 static/scss/components/_article.scss:33: #no-content-for-platform-message On 2017/11/15 16:57:30, ire wrote: > I just ...
4 weeks ago (2017-11-15 18:03:33 UTC) #5
ire
https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/components/_article.scss File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/components/_article.scss#newcode33 static/scss/components/_article.scss:33: #no-content-for-platform-message On 2017/11/15 18:03:33, juliandoucette wrote: > On 2017/11/15 ...
2 weeks, 2 days ago (2017-11-27 18:26:45 UTC) #6
juliandoucette
On 2017/11/27 18:26:45, ire wrote: > I like your suggestions. I think we should move ...
2 weeks ago (2017-11-29 20:50:19 UTC) #7
ire
On 2017/11/29 20:50:19, juliandoucette wrote: > On 2017/11/27 18:26:45, ire wrote: > > I like ...
1 week, 3 days ago (2017-12-04 13:27:24 UTC) #8
ire
On 2017/12/04 13:27:24, ire wrote: > On 2017/11/29 20:50:19, juliandoucette wrote: > > On 2017/11/27 ...
1 week, 2 days ago (2017-12-05 09:37:06 UTC) #9
juliandoucette
Thanks Ire! https://codereview.adblockplus.org/29607560/diff/29630601/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29607560/diff/29630601/static/js/main.js#newcode329 static/js/main.js:329: this.noContentMessage.setAttribute("hidden", "true"); Why are you setting and ...
1 week, 2 days ago (2017-12-05 16:38:53 UTC) #10
ire
https://codereview.adblockplus.org/29607560/diff/29630601/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29607560/diff/29630601/static/js/main.js#newcode329 static/js/main.js:329: this.noContentMessage.setAttribute("hidden", "true"); On 2017/12/05 16:38:52, juliandoucette wrote: > Why ...
1 week, 1 day ago (2017-12-06 14:43:39 UTC) #11
juliandoucette
LGTM + NITs https://codereview.adblockplus.org/29607560/diff/29630601/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29607560/diff/29630601/static/js/main.js#newcode329 static/js/main.js:329: this.noContentMessage.setAttribute("hidden", "true"); On 2017/12/06 14:43:39, ire ...
3 days, 1 hour ago (2017-12-11 15:15:29 UTC) #12
ire
Thanks! Rebased & Addressed NITs https://codereview.adblockplus.org/29607560/diff/29630601/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29607560/diff/29630601/static/js/main.js#newcode361 static/js/main.js:361: var browser = event.target.parentElement.className; ...
2 days, 6 hours ago (2017-12-12 10:21:14 UTC) #13
juliandoucette
Followup answers. https://codereview.adblockplus.org/29607560/diff/29631606/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29607560/diff/29631606/static/js/main.js#newcode270 static/js/main.js:270: var supportedBrowser = this.SUPPORTED_BROWSERS[i]; On 2017/12/12 10:21:13, ...
2 days, 4 hours ago (2017-12-12 12:30:31 UTC) #14
ire
Thanks Julian. Responses: https://codereview.adblockplus.org/29607560/diff/29631606/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29607560/diff/29631606/static/js/main.js#newcode270 static/js/main.js:270: var supportedBrowser = this.SUPPORTED_BROWSERS[i]; Thanks for ...
1 day, 6 hours ago (2017-12-13 10:30:19 UTC) #15
juliandoucette
1 day, 1 hour ago (2017-12-13 14:57:48 UTC) #16
Message was sent while issue was closed.
https://codereview.adblockplus.org/29607560/diff/29631606/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29607560/diff/29631606/static/js/main.js#n...
static/js/main.js:270: var supportedBrowser = this.SUPPORTED_BROWSERS[i];
On 2017/12/13 10:30:18, ire wrote:
> I don't see the purpose of separating the variable declaration and assignment
> (more lines?). 

To avoid confusion about the scope of the variable.

> When using for..in loops with Arrays, the variable is actually the key, so it
> would actually be `for (var index in browsers)`. Plus, I read that using
for..in
> loops with Arrays isn't advisable since it doesn't always happen in order
> (although I realise that's not important in this case) 

Ack.

> This would probably have been the best way to do it. I have generally stayed
> away from forEach because of IE8 but since we aren't "officially" supporting
IE8
> then I guess I could have used it. 

Ack/Agreed.

https://codereview.adblockplus.org/29607560/diff/29631606/static/js/main.js#n...
static/js/main.js:271: if
(!document.querySelector(".platform-"+supportedBrowser))
On 2017/12/13 10:30:18, ire wrote:
> Ah I see! I think your solution is preferable. I may go back and change it but
I
> don't think it is currently worth having to change all the
`platform-${browser}`
> selectors to work the other way around. 

Agreed.
Sign in to reply to this message.

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