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

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

[ire, 2017-11-03 10:24:35]

I think we need to style the no-content-for-platform-message better. Maybe in a
separate issue. Will ask Jeen about it.

https://codereview.adblockplus.org/29596595/diff/29596596/globals/browsers.py
File globals/browsers.py (right):

https://codereview.adblockplus.org/29596595/diff/29596596/globals/browsers.py...
globals/browsers.py:35: 'id': 'msie',
I forgot to change these before

[juliandoucette, 2017-11-06 10:45:01]

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:324: document
Why insert this instead of adding a class to the body in order to show the
no-content-for-platform-message like any other section?

[ire, 2017-11-06 14:27:20]

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:324: document
On 2017/11/06 10:45:00, juliandoucette wrote:
> Why insert this instead of adding a class to the body in order to show the
> no-content-for-platform-message like any other section?

I think it makes sense semantically this way. Because the message *is* the
content for that platform. For example we may want to personalise the message
for each browser, e.g. "There's no content for ${browser}". The way I see it,
the only reason it isn't already in the markup is because of no-js/crawlers. 

Also I don't see the no-content-for-platform-message as a section by itself. So
I don't think I would want a class in the body to display it. I would reserve
those classes to only display the user agent, not to determine if there is
content for that user agent or not.

[juliandoucette, 2017-11-06 15:56:05]

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#n...
static/js/main.js:313: BrowserSelect.prototype.checkBrowserContent =
function(browser)
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?

https://codereview.adblockplus.org/29596595/diff/29596596/static/js/main.js#n...
static/js/main.js:318: var section = document.createElement("section");
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)

[ire, 2017-11-08 07:47:00]

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.