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

Issue 29596595: Issue 5912 - Hide/Show Content based on selected browser on help.eyeo.com article pages (Closed)

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

Description

Issue 5912 - Hide/Show Content based on selected browser on help.eyeo.com article pages

Patch Set 1 #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+46 lines, -4 lines) Patch
M globals/browsers.py View 1 chunk +7 lines, -3 lines 1 comment Download
M static/js/main.js View 1 chunk +18 lines, -0 lines 6 comments Download
M static/scss/base/_variables.scss View 1 chunk +6 lines, -1 line 0 comments Download
M static/scss/components/_article.scss View 1 chunk +15 lines, -0 lines 0 comments Download

Messages

Total messages: 6
ire
1 year, 8 months ago (2017-11-03 10:23:16 UTC) #1
ire
I think we need to style the no-content-for-platform-message better. Maybe in a separate issue. Will ...
1 year, 8 months ago (2017-11-03 10:24:35 UTC) #2
juliandoucette
https://codereview.adblockplus.org/29596595/diff/29596596/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29596595/diff/29596596/static/js/main.js#newcode324 static/js/main.js:324: document Why insert this instead of adding a class ...
1 year, 8 months ago (2017-11-06 10:45:01 UTC) #3
ire
https://codereview.adblockplus.org/29596595/diff/29596596/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29596595/diff/29596596/static/js/main.js#newcode324 static/js/main.js:324: document On 2017/11/06 10:45:00, juliandoucette wrote: > Why insert ...
1 year, 8 months ago (2017-11-06 14:27:20 UTC) #4
juliandoucette
LGTM + NITs https://codereview.adblockplus.org/29596595/diff/29596596/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29596595/diff/29596596/static/js/main.js#newcode313 static/js/main.js:313: BrowserSelect.prototype.checkBrowserContent = function(browser) NIT: I don't ...
1 year, 8 months ago (2017-11-06 15:56:05 UTC) #5
ire
1 year, 8 months ago (2017-11-08 07:47:00 UTC) #6
https://codereview.adblockplus.org/29596595/diff/29596596/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29596595/diff/29596596/static/js/main.js#n...
static/js/main.js:313: BrowserSelect.prototype.checkBrowserContent =
function(browser)
On 2017/11/06 15:56:05, juliandoucette wrote:
> NIT: I don't think that this method name is descriptive enough. Check browser
> content for what? What does it do? What does it return? I don't know from the
> name. Do you know what I mean?

I think you're right.

Since the method was essentially do 2 things (1. checking if there is content
and 2. handling the no content state), I decided to remove the first to the
previous function, and only call this function if there is no content.

Renamed to handleNoContentForBrowser()

https://codereview.adblockplus.org/29596595/diff/29596596/static/js/main.js#n...
static/js/main.js:318: var section = document.createElement("section");
On 2017/11/06 15:56:05, juliandoucette wrote:
> TOL: This could be done with fewer lines of code e.g.
> 
> document.getElementById("tutorial").innerHTML += 
>   document.getElementById("not-supported-message").innerHTML
>     .replace("${browser}", browser);
> 
> If...
> - The <section> was in the template
> - The article body had an id (e.g. tutorial)
> - It doesn't matter where the unsupported message is inserted (beginning or
end)

I think it does matter where the unsupported message is inserted. Since it
explicitly says "use the dropdown menu above to choose a different browser", it
makes sense to place it directly below the dropdown menu.

I also need to add a class to the <section> depending on what browser it is.

Using `innerHTML +=` isn't great performance-wise because it causes the entire
innerHTML of the element to be reloaded. Using `insertAdjacentHTML` will only
affect the particular position the element is being written to.
Sign in to reply to this message.

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