Issue 29607560: Issue 6003 - Better handling of no-content-for-platform-message on help.eyeo.com (Closed)
|
Created:
Nov. 14, 2017, noon by ire Modified:
Dec. 13, 2017, 2:57 p.m. Reviewers:
juliandoucette Base URL:
https://hg.adblockplus.org/help.eyeo.com Visibility:
Public. |
DescriptionIssue 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 #
MessagesTotal messages: 16
Ready for review https://codereview.adblockplus.org/29607560/diff/29607561/static/scss/compone... File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29607561/static/scss/compone... static/scss/components/_article.scss:33: #no-content-for-platform-message I thought it looked better that this section also has a border-bottom. This was a personal decision https://codereview.adblockplus.org/29607560/diff/29607561/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29607560/diff/29607561/static/scss/content... static/scss/content/_typography.scss:60: { This should probably be in website-defaults https://codereview.adblockplus.org/29607560/diff/29607561/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29607560/diff/29607561/templates/article.t... templates/article.tmpl:40: <div id="no-content-for-platform-message" class="content" aria-hidden="true" hidden> I didn't think it made sense to keep this a template anymore since I was just hiding/showing.
https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/compone... File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/compone... static/scss/components/_article.scss:33: #no-content-for-platform-message NIT: Why should this article content have a border-bottom; unlike other article content? https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/content... static/scss/content/_typography.scss:59: .btn-link NIT: "btn" is inconsistent with our other names that are spelled out https://codereview.adblockplus.org/29607560/diff/29608560/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29607560/diff/29608560/templates/article.t... templates/article.tmpl:40: <div id="no-content-for-platform-message" class="content" aria-hidden="true" hidden> I think that aria-hidden and hidden is redundant. Source: https://developer.paciellogroup.com/blog/2012/05/html5-accessibility-chops-hi... https://codereview.adblockplus.org/29607560/diff/29608560/templates/article.t... templates/article.tmpl:41: <p>{{ "Unfortunately, there is no content specific to your current browser. Please choose a different browser from the list below:" | translate("no-content-for-platform-message", "paragraph") }}</p> NIT: This message doesn't make sense when you select a browser that there is no content for
https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/compone... File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/compone... static/scss/components/_article.scss:33: #no-content-for-platform-message On 2017/11/15 13:27:44, juliandoucette wrote: > NIT: Why should this article content have a border-bottom; unlike other article > content? I guess I don't really see it as article content. I see it as a separate message, which is also why I removed it from the .article-body . But as I mentioned, this is totally a personal suggestion (Jeen didn't ask me to add this). I just thought it looked better being separated from the rest of the content. https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/content... static/scss/content/_typography.scss:59: .btn-link On 2017/11/15 13:27:45, juliandoucette wrote: > NIT: "btn" is inconsistent with our other names that are spelled out Changed to .button-link https://codereview.adblockplus.org/29607560/diff/29608560/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29607560/diff/29608560/templates/article.t... templates/article.tmpl:40: <div id="no-content-for-platform-message" class="content" aria-hidden="true" hidden> On 2017/11/15 13:27:45, juliandoucette wrote: > I think that aria-hidden and hidden is redundant. > > Source: > https://developer.paciellogroup.com/blog/2012/05/html5-accessibility-chops-hi... You're right. I wasn't sure about this myself. Thanks for sharing the article! Done. https://codereview.adblockplus.org/29607560/diff/29608560/templates/article.t... templates/article.tmpl:41: <p>{{ "Unfortunately, there is no content specific to your current browser. Please choose a different browser from the list below:" | translate("no-content-for-platform-message", "paragraph") }}</p> On 2017/11/15 13:27:45, juliandoucette wrote: > NIT: This message doesn't make sense when you select a browser that there is no > content for Why doesn't it make sense? And do you have a suggestion for something different?
https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/compone... File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/compone... static/scss/components/_article.scss:33: #no-content-for-platform-message On 2017/11/15 16:57:30, ire wrote: > I just thought it looked better being separated from the rest of the content. Does it ever not replace the article content as the main content in that area? https://codereview.adblockplus.org/29607560/diff/29608560/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29607560/diff/29608560/templates/article.t... templates/article.tmpl:41: <p>{{ "Unfortunately, there is no content specific to your current browser. Please choose a different browser from the list below:" | translate("no-content-for-platform-message", "paragraph") }}</p> On 2017/11/15 16:57:30, ire wrote: > Why doesn't it make sense? "Your current browser" means "the browser that you are using" AFAICT. Which doesn't make sense (is incorrect) if the selected browser is not the browser that you are using. Furthermore, this statement is ambiguous because "content" is general and is not related to the subject/question in this statement. e.g. "there is no content specific to your context" vs "there is no *answer* to *your question* in/for *your context*" ... if you know what I mean? > And do you have a suggestion for something different? I hate writing... D: I can suggest several elements: 1. Oops sorry 2. We don't have (an answer | a tutorial) (to $THIS_QUESTION | for $THIS_SUBJECT) for $THIS_BROWSER 3. Point to browsers that we do have (an answer | a tutorial) for 4. Point at Popular Topics section (may not be necessary) 5. Point at Still looking for help section (may not be necessary) "Oops! There is no tutorial for the subject and browser that you have selected. Please find a list of browsers that this tutorial is available for below. "
https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/compone... File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29608560/static/scss/compone... static/scss/components/_article.scss:33: #no-content-for-platform-message On 2017/11/15 18:03:33, juliandoucette wrote: > On 2017/11/15 16:57:30, ire wrote: > > I just thought it looked better being separated from the rest of the content. > > Does it ever not replace the article content as the main content in that area? Yes. There are cases where there is "general" content that isn't within a `.platform-${browser}` container, so that shows even when there is no content for the specific browser https://codereview.adblockplus.org/29607560/diff/29608560/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29607560/diff/29608560/templates/article.t... templates/article.tmpl:41: <p>{{ "Unfortunately, there is no content specific to your current browser. Please choose a different browser from the list below:" | translate("no-content-for-platform-message", "paragraph") }}</p> On 2017/11/15 18:03:33, juliandoucette wrote: > On 2017/11/15 16:57:30, ire wrote: > > Why doesn't it make sense? > > "Your current browser" means "the browser that you are using" AFAICT. Which > doesn't make sense (is incorrect) if the selected browser is not the browser > that you are using. > > Furthermore, this statement is ambiguous because "content" is general and is not > related to the subject/question in this statement. e.g. "there is no content > specific to your context" vs "there is no *answer* to *your question* in/for > *your context*" ... if you know what I mean? Ack. I understand. > > And do you have a suggestion for something different? > > I hate writing... D: > > I can suggest several elements: > > 1. Oops sorry > 2. We don't have (an answer | a tutorial) (to $THIS_QUESTION | for > $THIS_SUBJECT) for $THIS_BROWSER > 3. Point to browsers that we do have (an answer | a tutorial) for > 4. Point at Popular Topics section (may not be necessary) > 5. Point at Still looking for help section (may not be necessary) > > "Oops! There is no tutorial for the subject and browser that you have selected. > Please find a list of browsers that this tutorial is available for below. " I like your suggestions. I think we should move this discussion outside this review (back to the issue) so we can get other feedback on this as well. I'll copy this discussion over there now
On 2017/11/27 18:26:45, ire wrote: > I like your suggestions. I think we should move this discussion outside this > review (back to the issue) so we can get other feedback on this as well. I'll > copy this discussion over there now Thanks. Please reply here when you receive an answer.
On 2017/11/29 20:50:19, juliandoucette wrote: > On 2017/11/27 18:26:45, ire wrote: > > I like your suggestions. I think we should move this discussion outside this > > review (back to the issue) so we can get other feedback on this as well. I'll > > copy this discussion over there now > > Thanks. > > Please reply here when you receive an answer. See https://issues.adblockplus.org/ticket/6003#comment:9 Will update this issue soon.
On 2017/12/04 13:27:24, ire wrote: > On 2017/11/29 20:50:19, juliandoucette wrote: > > On 2017/11/27 18:26:45, ire wrote: > > > I like your suggestions. I think we should move this discussion outside this > > > review (back to the issue) so we can get other feedback on this as well. > I'll > > > copy this discussion over there now > > > > Thanks. > > > > Please reply here when you receive an answer. > > See https://issues.adblockplus.org/ticket/6003#comment:9 > > Will update this issue soon. New patch set uploaded.
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#n... static/js/main.js:329: this.noContentMessage.setAttribute("hidden", "true"); Why are you setting and then conditionally removing this attribute instead of *just* conditionally setting it? https://codereview.adblockplus.org/29607560/diff/29630601/static/js/main.js#n... static/js/main.js:361: var browser = event.target.parentElement.className; suggest: use a data value e.g. data-browser instead of a class name - because this is "data" and that's what "data attributes" are for - because classes can be / are often modified by separate scripts (generally, not particularly on abp.org) https://codereview.adblockplus.org/29607560/diff/29630601/static/scss/compone... File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29630601/static/scss/compone... static/scss/components/_article.scss:31: .article-heading, TOL/suggest: A more descriptive class name that is extended by .article-heading, .article-browser-selector etc
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#n... static/js/main.js:329: this.noContentMessage.setAttribute("hidden", "true"); On 2017/12/05 16:38:52, juliandoucette wrote: > Why are you setting and then conditionally removing this attribute instead of > *just* conditionally setting it? Since this is the default I wanted to always set it and only unhide it. But I can move this to the else part of the if statement. https://codereview.adblockplus.org/29607560/diff/29630601/static/js/main.js#n... static/js/main.js:329: this.noContentMessage.setAttribute("hidden", "true"); On 2017/12/05 16:38:52, juliandoucette wrote: > Why are you setting and then conditionally removing this attribute instead of > *just* conditionally setting it? Done. https://codereview.adblockplus.org/29607560/diff/29630601/static/js/main.js#n... static/js/main.js:361: var browser = event.target.parentElement.className; On 2017/12/05 16:38:52, juliandoucette wrote: > suggest: use a data value e.g. data-browser instead of a class name > > - because this is "data" and that's what "data attributes" are for > - because classes can be / are often modified by separate scripts (generally, > not particularly on http://abp.org) I agree with you. Done. For some reason (which I can't figure out right now) I used the name `data-value` in the browser select dropdown, so I'm using it here as well. I should have called it `data-browser` and might change it in a different issue https://codereview.adblockplus.org/29607560/diff/29630601/static/scss/compone... File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29630601/static/scss/compone... static/scss/components/_article.scss:31: .article-heading, On 2017/12/05 16:38:52, juliandoucette wrote: > TOL/suggest: A more descriptive class name that is extended by .article-heading, > .article-browser-selector etc Since these particular styles aren't used anywhere else, I don't think it would be particularly more helpful to create a separate class that is only extended here once.
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#n... static/js/main.js:329: this.noContentMessage.setAttribute("hidden", "true"); On 2017/12/06 14:43:39, ire wrote: > On 2017/12/05 16:38:52, juliandoucette wrote: > > Why are you setting and then conditionally removing this attribute instead of > > *just* conditionally setting it? > > Since this is the default I wanted to always set it and only unhide it. But I > can move this to the else part of the if statement. Acknowledged. https://codereview.adblockplus.org/29607560/diff/29630601/static/js/main.js#n... static/js/main.js:361: var browser = event.target.parentElement.className; On 2017/12/06 14:43:39, ire wrote: > On 2017/12/05 16:38:52, juliandoucette wrote: > > suggest: use a data value e.g. data-browser instead of a class name > > > > - because this is "data" and that's what "data attributes" are for > > - because classes can be / are often modified by separate scripts (generally, > > not particularly on http://abp.org) > > I agree with you. Done. > > For some reason (which I can't figure out right now) I used the name > `data-value` in the browser select dropdown, so I'm using it here as well. I > should have called it `data-browser` and might change it in a different issue I think data-value makes sense in the dropdown because it emulates option.value. I don't think that it makes sense on an anchor for the purpose of selecting though because the "value like" property of an achor is it's href. https://codereview.adblockplus.org/29607560/diff/29630601/static/scss/compone... File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29630601/static/scss/compone... static/scss/components/_article.scss:31: .article-heading, On 2017/12/06 14:43:39, ire wrote: > On 2017/12/05 16:38:52, juliandoucette wrote: > > TOL/suggest: A more descriptive class name that is extended by > .article-heading, > > .article-browser-selector etc > > Since these particular styles aren't used anywhere else, I don't think it would > be particularly more helpful to create a separate class that is only extended > here once. Acknowledged. 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]; NIT: Despite "Declare local variables as near to their use as possible." [1] I would argue "JavaScript does not have block scope, so defining variables in blocks can confuse programmers who are experienced with other C family languages. Define all variables at the top of the function." [2] (Acknowledging that ES6 *does* have block scope) (Acknowledging that this may be an aging generational preference.) [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... [2] http://crockford.com/javascript/#variable%20declarations https://codereview.adblockplus.org/29607560/diff/29631606/static/js/main.js#n... static/js/main.js:271: if (!document.querySelector(".platform-"+supportedBrowser)) NIT/TOL: querySelector supports selector groups e.g. ".platform-chrome, .platform-firefox". Therefore, it may be more efficient to explode SUPPORTED_BROWSERS into a selector and perform one query than performing each query individually. e.g. querySelector(browsers.join('-platform, ')) //works with -platform suffix, not prefix Detail: The performance impact will be negligible in this case. Therefore this is more of a SuperNIT programming style issue. And I chose to point it out instead of ignoring it in-case you like this solution personally or learn something new e.g. that selector groups are a thing. https://codereview.adblockplus.org/29607560/diff/29631606/static/js/main.js#n... static/js/main.js:363: var browser = event.target.parentElement.getAttribute("data-value"); NIT: Unnecessary variable. this.selectOption( event.target.parentElement.getAttribute("data-value") ); https://codereview.adblockplus.org/29607560/diff/29631606/static/scss/compone... File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29631606/static/scss/compone... static/scss/components/_article.scss:35: @extend .underlined; TOL: lol (you extended in a different way than I suggested) (not a mistake, just a funny-to-me coincidence). https://codereview.adblockplus.org/29607560/diff/29631606/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29607560/diff/29631606/static/scss/content... static/scss/content/_typography.scss:59: .button-link TOL: I think we were looking for this in our last code review meeting. https://codereview.adblockplus.org/29607560/diff/29631606/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29607560/diff/29631606/templates/article.t... templates/article.tmpl:41: <p>{{ "Oops! There is no article for the subject and browser that you have selected. Please find a list of browsers that this article is available for below." | translate("no-content-for-platform-message", "paragraph") }}</p> NIT/TOL: This is arguably more of a "notice" than a paragraph https://codereview.adblockplus.org/29607560/diff/29631606/templates/article.t... templates/article.tmpl:41: <p>{{ "Oops! There is no article for the subject and browser that you have selected. Please find a list of browsers that this article is available for below." | translate("no-content-for-platform-message", "paragraph") }}</p> TOL: The paragraph description is arguably the ~default description and may be unnecessary.
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#n... static/js/main.js:361: var browser = event.target.parentElement.className; On 2017/12/11 15:15:27, juliandoucette wrote: > On 2017/12/06 14:43:39, ire wrote: > > On 2017/12/05 16:38:52, juliandoucette wrote: > > > suggest: use a data value e.g. data-browser instead of a class name > > > > > > - because this is "data" and that's what "data attributes" are for > > > - because classes can be / are often modified by separate scripts > (generally, > > > not particularly on http://abp.org) > > > > I agree with you. Done. > > > > For some reason (which I can't figure out right now) I used the name > > `data-value` in the browser select dropdown, so I'm using it here as well. I > > should have called it `data-browser` and might change it in a different issue > > I think data-value makes sense in the dropdown because it emulates option.value. > I don't think that it makes sense on an anchor for the purpose of selecting > though because the "value like" property of an achor is it's href. Acknowledged. 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/11 15:15:28, juliandoucette wrote: > NIT: Despite "Declare local variables as near to their use as possible." [1] I > would argue "JavaScript does not have block scope, so defining variables in > blocks can confuse programmers who are experienced with other C family > languages. Define all variables at the top of the function." [2] (Acknowledging > that ES6 *does* have block scope) (Acknowledging that this may be an aging > generational preference.) > > [1] > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... > > [2] http://crockford.com/javascript/#variable%20declarations I'm not sure I understand where else this variable would go? It is dependent on the variable i in the line before and used in the line immediately after. 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/11 15:15:28, juliandoucette wrote: > NIT/TOL: querySelector supports selector groups e.g. ".platform-chrome, > .platform-firefox". Therefore, it may be more efficient to explode > SUPPORTED_BROWSERS into a selector and perform one query than performing each > query individually. > > e.g. querySelector(browsers.join('-platform, ')) //works with -platform suffix, > not prefix > > Detail: The performance impact will be negligible in this case. Therefore this > is more of a SuperNIT programming style issue. And I chose to point it out > instead of ignoring it in-case you like this solution personally or learn > something new e.g. that selector groups are a thing. Thanks! I did know of selector groups but didn't think of it in this case. I don't think this particular method could be used here since I am checking for the absence of a particular selector, so I may still need to perform the loop anyway. But thanks for pointing this out, it's at least brought this method back into my consciousness for another use. https://codereview.adblockplus.org/29607560/diff/29631606/static/js/main.js#n... static/js/main.js:363: var browser = event.target.parentElement.getAttribute("data-value"); On 2017/12/11 15:15:28, juliandoucette wrote: > NIT: Unnecessary variable. > > this.selectOption( > event.target.parentElement.getAttribute("data-value") > ); Done. https://codereview.adblockplus.org/29607560/diff/29631606/static/scss/compone... File static/scss/components/_article.scss (right): https://codereview.adblockplus.org/29607560/diff/29631606/static/scss/compone... static/scss/components/_article.scss:35: @extend .underlined; On 2017/12/11 15:15:28, juliandoucette wrote: > TOL: lol (you extended in a different way than I suggested) (not a mistake, just > a funny-to-me coincidence). Haha :p https://codereview.adblockplus.org/29607560/diff/29631606/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29607560/diff/29631606/static/scss/content... static/scss/content/_typography.scss:59: .button-link On 2017/12/11 15:15:28, juliandoucette wrote: > TOL: I think we were looking for this in our last code review meeting. Oh yeah! I'll remove this when that issue is committed https://codereview.adblockplus.org/29607560/diff/29631606/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29607560/diff/29631606/templates/article.t... templates/article.tmpl:41: <p>{{ "Oops! There is no article for the subject and browser that you have selected. Please find a list of browsers that this article is available for below." | translate("no-content-for-platform-message", "paragraph") }}</p> On 2017/12/11 15:15:29, juliandoucette wrote: > NIT/TOL: This is arguably more of a "notice" than a paragraph Ack. I think "notice" works. https://codereview.adblockplus.org/29607560/diff/29631606/templates/article.t... templates/article.tmpl:41: <p>{{ "Oops! There is no article for the subject and browser that you have selected. Please find a list of browsers that this article is available for below." | translate("no-content-for-platform-message", "paragraph") }}</p> On 2017/12/11 15:15:29, juliandoucette wrote: > NIT/TOL: This is arguably more of a "notice" than a paragraph Done.
Message was sent while issue was closed.
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#n... static/js/main.js:270: var supportedBrowser = this.SUPPORTED_BROWSERS[i]; On 2017/12/12 10:21:13, ire wrote: > On 2017/12/11 15:15:28, juliandoucette wrote: > > NIT: Despite "Declare local variables as near to their use as possible." [1] I > > would argue "JavaScript does not have block scope, so defining variables in > > blocks can confuse programmers who are experienced with other C family > > languages. Define all variables at the top of the function." [2] > (Acknowledging > > that ES6 *does* have block scope) (Acknowledging that this may be an aging > > generational preference.) > > > > [1] > > > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... > > > > [2] http://crockford.com/javascript/#variable%20declarations > > I'm not sure I understand where else this variable would go? It is dependent on > the variable i in the line before and used in the line immediately after. Traditionally... ``` var name; for (...) { name = names[i]; ... ``` But you could also... ``` for (var name in browsers) { ... ``` Or... ``` browsers.forEach(function(name){ ... ``` 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/12 10:21:13, ire wrote: > I don't think this particular method could be used here since I am checking for > the absence of a particular selector, so I may still need to perform the loop > anyway. If a selector in a selector group matches no results then no results from that selector are added to the resulting nodelist. As a result, when you loop *just* the matching results, you are performing fewer iterations (where you in turn perform one fewer condition and one fewwer DOM query).
Message was sent while issue was closed.
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#n... static/js/main.js:270: var supportedBrowser = this.SUPPORTED_BROWSERS[i]; Thanks for the suggestions. Responses: On 2017/12/12 12:30:31, juliandoucette wrote: > Traditionally... > > ``` > var name; > > for (...) { > name = names[i]; > ... > ``` I don't see the purpose of separating the variable declaration and assignment (more lines?). > But you could also... > > ``` > for (var name in browsers) { > ... > ``` 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) Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/... > Or... > > ``` > browsers.forEach(function(name){ > ... > ``` 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. 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/12 12:30:31, juliandoucette wrote: > On 2017/12/12 10:21:13, ire wrote: > > I don't think this particular method could be used here since I am checking > for > > the absence of a particular selector, so I may still need to perform the loop > > anyway. > > If a selector in a selector group matches no results then no results from that > selector are added to the resulting nodelist. As a result, when you loop *just* > the matching results, you are performing fewer iterations (where you in turn > perform one fewer condition and one fewwer DOM query). 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.
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. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
