Issue 29488575: Issue 5384 - Introduced dedicated mobile options page (Closed)
|
Created:
July 13, 2017, 2:54 p.m. by Thomas Greiner Modified:
Aug. 28, 2017, 4:22 p.m. Reviewers:
saroyanm Visibility:
Public. |
DescriptionIssue 5384 - Introduced dedicated mobile options page
Patch Set 1 #
Total comments: 46
Patch Set 2 : Encapsulated mobile-options.js script #Patch Set 3 : Addressed mobile-options.js comments #
Total comments: 55
Patch Set 4 : #
Total comments: 30
Patch Set 5 : Fixed linter errors #Patch Set 6 : Rebased to master #Patch Set 7 : Removed menu item and fixed new linter error #Patch Set 8 : Added ID constants #
MessagesTotal messages: 18
Thanks Thomas for uploading the review, As it's bit bigger than I expected, I only managed to review the mobile-options.js yet. I'll continue reviewing other files next week, but I thought to provide you something so you can already implement. I'll be on vacation, but if you will have questions about my comments and be blocked, please let me know (Write an email or schedule a call I'll be sure to get online) so we can discuss my comments. Most of them are details and are optional, but there are also important one, like Acceptable Ads not functioning because of ID change. Anyway all important comments should be easy to fix I guess. https://codereview.adblockplus.org/29488575/diff/29488576/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29488575/diff/29488576/messageResponder.js... messageResponder.js:200: { Why is this change necessary ? https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.html File mobile-options.html (right): https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.html... mobile-options.html:40: <label class="toggle-image" for="enabled"></label> The toggle is invisible, Seems like the checkbox image path needs to be updated. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:21: I'll suggest to encapsulate this file with own block scope, to ensure that global function names will not have conflict in future with other files. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:29: function get(selector, origin) Detail: Calling function "get" is too generic, as we are getting an element, maybe we can call this "getElement", "getAllElements" ? Anyway this shouldn't be much of a problem if we enclose the file in it's scope. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:62: }); Note: I do remember before it was required to specify useCapture. I couldn't find that rule in Coding styles anymore. Anyway while you are using stopPropagation, it shouldn't matter anyway. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:93: function getRecommended() This function only return recommended subscriptions of type ads. Suggestion: what about renaming this function to "getRecommendedAds". https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:96: .then((resp) => resp.text()) Shouldn't this be returned (return response.text();) ? In order to be used in next chain ? (Maybe I'm missing something here) https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:96: .then((resp) => resp.text()) Detail: we usually are not using short form of the words. ex. "response" instead "resp". (exception "ev", for event ?) Same applies to other occurrences ex. "doc". https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:106: return { Detail: the brace should go to the next row for consistency and readability I assume. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:107: title: element.getAttribute("title"), Shouldn't this titles be translated ? I think we should somehow provide translations, or include the name of the language in the native language (ex. next to the language or below). https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:140: let installed = get(`[data-url="${url}"]`, listInstalled); Detail: I think we are using single quotes "'", rather than back-quotes "`" for strings. (Same applies to other occurrences) https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:147: else if (shouldAdd) Why are you using "shouldAdd" parameter ? If the subscription is not installed and is not disabled, then it should be added, if I'm not missing something here. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:170: get("#acceptable-ads").checked = false; I think you forget to change all camelCase IDs. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:206: function setError(dialogId, message) Detail: Parameter name "message" is misleading, in this case I think you are passing "fieldName" rather than "message", which feels to be error message, while it's not. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:233: setDialog("subscribe", {title, url}); Detail: reference to "subscribe" dialog is used quite often, might make sense to use a separate variable for that. Also the variable can be used to generate the actual id. ex.: "dialog-" + dialogName. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:246: .catch((err) => console.error(err)); I think you have mentioned this, so I'll ignore that this error is not visible in the UI as it's also not specified in the specifications. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:253: if (ev.target.id != "acceptable-ads") Same as mentioned above, there is no element with "acceptable-ads" ID. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:274: case "close-dialog": Note: Why not to handle all other click events here as well ? Ex. Remove subscription, add subscription and etc. (If it's appears to be non trivial, this solution is still fine for the release) https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:310: let checkbox = ev.target; Suggestion: We can avoid using element name as a variable, as the element itself can change (I don't have strong opinion regarding this, but I do remember you mentioned similar case in another review) https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:343: whitelistFilter = `@@||${host}^$document`; Shouldn't this be promise as well, similar to the "promisedAcceptableAdsUrl" ? (If non trivial, this solution still looks fine to me, I don't have strong opinion about this) https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:362: if (!whitelistFilter || filter.text != whitelistFilter) First check for "!whitelistFilter" seems to redundant until you don't expect "filter.text" to be undefined or null, in case of undefined hardequation should be enough "filter.text !== whitelistFilter" I guess.
Thanks for the quick feedback and enjoy your vacation! In the meantime I uploaded two new patch sets to address your comments so far. https://codereview.adblockplus.org/29488575/diff/29488576/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29488575/diff/29488576/messageResponder.js... messageResponder.js:200: { On 2017/07/18 12:46:26, saroyanm wrote: > Why is this change necessary ? This is how we pass the domain to the options page when opening it. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.html File mobile-options.html (right): https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.html... mobile-options.html:40: <label class="toggle-image" for="enabled"></label> On 2017/07/18 12:46:27, saroyanm wrote: > The toggle is invisible, Seems like the checkbox image path needs to be updated. Not sure what you mean. I see the toggle using http://localhost:5000/mobile-options.html?showPageOptions=true, I can see its image and I can interact with it. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:21: On 2017/07/18 12:46:28, saroyanm wrote: > I'll suggest to encapsulate this file with own block scope, to ensure that > global function names will not have conflict in future with other files. Done. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:29: function get(selector, origin) On 2017/07/18 12:46:28, saroyanm wrote: > Detail: Calling function "get" is too generic, as we are getting an element, > maybe we can call this "getElement", "getAllElements" ? > > Anyway this shouldn't be much of a problem if we enclose the file in it's scope. Acknowledged. Note that the inspiration for this function was the `E()` function in common.js because each of those utility functions could be moved into common.js if we want to. For now we wanted to avoid delays due to code reusage but that'd be an easy change to make afterwards if you agree that those functions would also be useful to have for other pages. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:62: }); On 2017/07/18 12:46:28, saroyanm wrote: > Note: I do remember before it was required to specify useCapture. I couldn't > find that rule in Coding styles anymore. Anyway while you are using > stopPropagation, it shouldn't matter anyway. We dropped that requirement a while ago. From what I remember, we had to specify it because of some Firefox issue and that we're no longer supporting said Firefox version. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:93: function getRecommended() On 2017/07/18 12:46:28, saroyanm wrote: > This function only return recommended subscriptions of type ads. > > Suggestion: what about renaming this function to "getRecommendedAds". Good point. Done. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:96: .then((resp) => resp.text()) On 2017/07/18 12:46:28, saroyanm wrote: > Shouldn't this be returned (return response.text();) ? In order to be used in > next chain ? > (Maybe I'm missing something here) No, this is one of the features of arrow functions. "In a concise body, only an expression is needed, and an implicit return is attached. In a block body, you must use an explicit return statement." https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/A... https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:96: .then((resp) => resp.text()) On 2017/07/18 12:46:27, saroyanm wrote: > Detail: we usually are not using short form of the words. ex. "response" instead > "resp". (exception "ev", for event ?) > > Same applies to other occurrences ex. "doc". I agree that it makes sense for "response", so I'll change that, but with others it helps us avoid confusion and issues due to existing variables. For instance, the "document" variable already exists and same applies for the "event" variable in the scope of an event listener. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:106: return { On 2017/07/18 12:46:27, saroyanm wrote: > Detail: the brace should go to the next row for consistency and readability I > assume. Moving this brace to the next row would be interpreted by the JavaScript engine as: return undefined; { title: element.getAttribute("title"), url: element.getAttribute("url") }; Secondly, this is not an opening brace but an object literal so the rule doesn't apply here. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:107: title: element.getAttribute("title"), On 2017/07/18 12:46:28, saroyanm wrote: > Shouldn't this titles be translated ? > > I think we should somehow provide translations, or include the name of the > language in the native language (ex. next to the language or below). We don't translate filter list titles (e.g. "EasyList") in any of our UIs. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:140: let installed = get(`[data-url="${url}"]`, listInstalled); On 2017/07/18 12:46:28, saroyanm wrote: > Detail: I think we are using single quotes "'", rather than back-quotes "`" for > strings. > > (Same applies to other occurrences) This is a template literal, not a string literal. It allows us to dynamically create a string without concatenation. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:147: else if (shouldAdd) On 2017/07/18 12:46:28, saroyanm wrote: > Why are you using "shouldAdd" parameter ? > > If the subscription is not installed and is not disabled, then it should be > added, if I'm not missing something here. Why should we show subscriptions that are not installed? https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:170: get("#acceptable-ads").checked = false; On 2017/07/18 12:46:27, saroyanm wrote: > I think you forget to change all camelCase IDs. Yep, seems so. Done. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:206: function setError(dialogId, message) On 2017/07/18 12:46:28, saroyanm wrote: > Detail: Parameter name "message" is misleading, in this case I think you are > passing "fieldName" rather than "message", which feels to be error message, > while it's not. Looks like I forgot to adapt that. Done. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:233: setDialog("subscribe", {title, url}); On 2017/07/18 12:46:27, saroyanm wrote: > Detail: reference to "subscribe" dialog is used quite often, might make sense to > use a separate variable for that. Done. > Also the variable can be used to generate the > actual id. ex.: "dialog-" + dialogName. Yes, that's how it's currently being used (see `setDialog()` and `setError()`) so not sure what you mean. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:246: .catch((err) => console.error(err)); On 2017/07/18 12:46:28, saroyanm wrote: > I think you have mentioned this, so I'll ignore that this error is not visible > in the UI as it's also not specified in the specifications. Acknowledged. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:253: if (ev.target.id != "acceptable-ads") On 2017/07/18 12:46:27, saroyanm wrote: > Same as mentioned above, there is no element with "acceptable-ads" ID. Done. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:274: case "close-dialog": On 2017/07/18 12:46:27, saroyanm wrote: > Note: Why not to handle all other click events here as well ? Ex. Remove > subscription, add subscription and etc. > > (If it's appears to be non trivial, this solution is still fine for the release) Done. I'm also using it now for the "enabled" toggle but it's simpler to do it this way for dynamically generated items because for those we'd need to determine which element was clicked and which element contains the information the action requires (e.g. title and URL). Therefore I left it that way for those actions. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:310: let checkbox = ev.target; On 2017/07/18 12:46:28, saroyanm wrote: > Suggestion: We can avoid using element name as a variable, as the element itself > can change (I don't have strong opinion regarding this, but I do remember you > mentioned similar case in another review) In this case the implementation depends on it being a checkbox. Of course we could still implement it using different HTML elements but we always expect it to behave like a checkbox. However, I guess "toggle" should be just as descriptive so I changed it to that one. Done. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:343: whitelistFilter = `@@||${host}^$document`; On 2017/07/18 12:46:28, saroyanm wrote: > Shouldn't this be promise as well, similar to the "promisedAcceptableAdsUrl" ? > (If non trivial, this solution still looks fine to me, I don't have strong > opinion about this) Even though it's not necessary because the UI using this value will only be initialized after the value has been set, a promise would indeed be the more appropriate solution. In this case, however, we'd need a deferred promise because we can only resolve the promise after we get the `host` value. Unfortunately, deferred promises have not been standardized yet so we'd have to implement that logic ourselves (see https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Prom...). Additionally, we don't even know whether this promise will ever resolve because the options page could be opened without referring to a domain. Therefore waiting for the value of the promise doesn't seem to make sense. Nevertheless, I added a check to `toggleWhitelistFilter()` now to ensure that dependencies are met. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:362: if (!whitelistFilter || filter.text != whitelistFilter) On 2017/07/18 12:46:28, saroyanm wrote: > First check for "!whitelistFilter" seems to redundant until you don't expect > "filter.text" to be undefined or null, in case of undefined hardequation should > be enough "filter.text !== whitelistFilter" I guess. While I don't expect `filter.text` to be `null`, it is theoretically possible so I'd rather avoid undefined behavior where possible. On that note, `null === null` is `true` so a strict comparison would not be sufficient to catch this case.
Thanks Thomas all your replies. I think I'll have another round of review today. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.html File mobile-options.html (right): https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.html... mobile-options.html:40: <label class="toggle-image" for="enabled"></label> On 2017/07/18 17:42:31, Thomas Greiner wrote: > On 2017/07/18 12:46:27, saroyanm wrote: > > The toggle is invisible, Seems like the checkbox image path needs to be > updated. > > Not sure what you mean. I see the toggle using > http://localhost:5000/mobile-options.html?showPageOptions=true, I can see its > image and I can interact with it. Interesting, the comment belongs to CSS file actually. I have assumption that you just forgot to remove "mobile/toggle.png" from your local test environment, while you use that image in CSS. I'll have a complete look into CSS file on next round. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:29: function get(selector, origin) On 2017/07/18 17:42:31, Thomas Greiner wrote: > On 2017/07/18 12:46:28, saroyanm wrote: > > Detail: Calling function "get" is too generic, as we are getting an element, > > maybe we can call this "getElement", "getAllElements" ? > > > > Anyway this shouldn't be much of a problem if we enclose the file in it's > scope. > > Acknowledged. > > Note that the inspiration for this function was the `E()` function in common.js > because each of those utility functions could be moved into common.js if we want > to. > > For now we wanted to avoid delays due to code reusage but that'd be an easy > change to make afterwards if you agree that those functions would also be useful > to have for other pages. Now I see your intention, but in that case I think maybe make sense to completely replace E() function with current solution ? Anyway while the file is encapsulated,current solution is fine, we can discuss replacing E() with current solution separately, as it's not a priority right now. https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.js#n... mobile-options.js:147: else if (shouldAdd) On 2017/07/18 17:42:31, Thomas Greiner wrote: > On 2017/07/18 12:46:28, saroyanm wrote: > > Why are you using "shouldAdd" parameter ? > > > > If the subscription is not installed and is not disabled, then it should be > > added, if I'm not missing something here. > > Why should we show subscriptions that are not installed? I think I meant "installed", nevermind, I don't remember why I made the comment. I think implementation is fine.
https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.html File mobile-options.html (right): https://codereview.adblockplus.org/29488575/diff/29488576/mobile-options.html... mobile-options.html:40: <label class="toggle-image" for="enabled"></label> On 2017/07/31 11:08:53, saroyanm wrote: > On 2017/07/18 17:42:31, Thomas Greiner wrote: > > On 2017/07/18 12:46:27, saroyanm wrote: > > > The toggle is invisible, Seems like the checkbox image path needs to be > > updated. > > > > Not sure what you mean. I see the toggle using > > http://localhost:5000/mobile-options.html?showPageOptions=true, I can see its > > image and I can interact with it. > > Interesting, the comment belongs to CSS file actually. I have assumption that > you just forgot to remove "mobile/toggle.png" from your local test environment, > while you use that image in CSS. I'll have a complete look into CSS file on next > round. As discussed, this is my mistake, I didn't download the PNG file.
Another round is done, I only managed to review CSS file, mostly is just comments about the color change according to Style guide. https://codereview.adblockplus.org/29488575/diff/29491653/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29491653/mobile-options.js#n... mobile-options.js:294: ev.preventDefault(); "ev" is not defined. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... File skin/mobile-options.css (right): https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:20: text-align: center; Detail: You are overwriting this value in child elements, see: main, [role="dialog"]. Alternatively you can specify "text-align: start;" here and remove the "main, [role="dialog"]" style completely. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:21: font-family: sans-serif; I think this is "Source Sans Pro". I think we can use similar declaration of Font-face as we use in desktop version -> https://hg.adblockplus.org/web.acceptableads.com/file/tip/static/css/main.css... https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:22: font-size: 14px; We use EMs and PXs all together, this solution is fine by me considering that in most cases EM is used for extending size without specifying exact size, but inheriting current one. We can also remove this size and calculate everything considering default 16px accordingly changing all font-sizes accordingly. But in general that would apply to also margins and all sizes to make it accessible and responsive. Not to complicate things we can launch with this solution, if we will have such requirement for improving accessibility and responsiveness we can update this accordingly later on. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:23: color: #3E4347; According to the style guide, I guess this should be "#4A4A4A" https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:70: color: #0A9BD0; I think this should be #5CBCE1 according to the style guide. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:98: border: 1px solid #BCBCBC; I think the border color should be "#BBBBBB" according to the style guide. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:98: border: 1px solid #BCBCBC; Newly added language in style guide, has also different border. While the style guide is not specified in mobile spec. I'll leave it up to you if you want to implement it or not, but if you will skip it should be fine for the launch IMO. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:105: flex: 1; I have assumption that you want to fill whole free space, I assume you want here flex: auto; ? As in example here -> https://developer.mozilla.org/en/docs/Web/CSS/flex https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:118: border-bottom: 1px solid #BCBCBC; The appearance of this element is different in desktop and mobile, I think make sense to follow the image provided in the Mobile version. So I think we just need to adapt the color which is #BBB I think. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:123: color: #9E9E9E; According to Style guide, this is #4A4A4A. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:144: flex-grow: 1; I'll suggest to use "flex" instead as it's done for "ul > li > span", for consistency reasons. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:155: flex-shrink: 0; Not sure if this is required. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:158: background-image: url(mobile/toggle.png); Detail: while we are using SVGs for other icons, I think it would be consistent to use SVGs for this as well, but I think you used PNG here as it's more convenient for switching the state. I can see pros and cons of both approach even I lean towards using SVG I'll leave it up to you to decide if you think it's better to use PNG here as well. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:169: height: 45px; Suggestion: let's use padding not to be dependent on font-size. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:173: color: #0A9BD0; I think this should be "#099CD0" according to the styleguide. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:189: border-color: #0A9BD0; "#5CBCE1" -styleguide. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:195: border-color: #BCBCBC; "#0A9DD1" - styleguide. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:203: background-color: #0A9BD0; Suggestion "#5CBCE1" for consistency, if the color is not set in SVG. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:210: border: 1px solid #BCBCBC; #BBB https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:232: max-width: 25em; Why are we using EMs here ? Other widths are specified in PXs. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:234: border: 1px solid #BCBCBC; #BBB https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:268: border-top: 1px solid #BCBCBC; #BBB https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:281: color: #D1142A; #C11D26 (Note: The visuals are different, but this one stands for RED color) https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:294: border-color: #D1142A; #C11D26 https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:309: max-height: calc(100vh - 40px); Why Calc is marked as experimental still ? https://developer.mozilla.org/en-US/docs/Web/CSS/calc I thought it's in CSS3 ? Alternatively we can use percentage ? https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:326: color: #A1A1A1; This is not specified, maybe: #BBB
https://codereview.adblockplus.org/29488575/diff/29491653/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29491653/mobile-options.js#n... mobile-options.js:294: ev.preventDefault(); On 2017/07/31 15:50:40, saroyanm wrote: > "ev" is not defined. Well spotted. It turned out to be a regression introduced by patch 3. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... File skin/mobile-options.css (right): https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:20: text-align: center; On 2017/07/31 15:50:43, saroyanm wrote: > Detail: You are overwriting this value in child elements, see: main, > [role="dialog"]. > Alternatively you can specify "text-align: start;" here and remove the "main, > [role="dialog"]" style completely. This is necessary for centering the main content and the dialog elements. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:21: font-family: sans-serif; On 2017/07/31 15:50:42, saroyanm wrote: > I think this is "Source Sans Pro". > I think we can use similar declaration of Font-face as we use in desktop version > -> > https://hg.adblockplus.org/web.acceptableads.com/file/tip/static/css/main.css... Done. I was just copying it from new-options.css. But I assume that you're also going to use "Source Sans Pro" there so I changed it now. Unlike with websites, we don't need to provide fallback fonts for other browsers or even older versions of Firefox Mobile so I only included the woff2 variants. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:22: font-size: 14px; On 2017/07/31 15:50:42, saroyanm wrote: > We use EMs and PXs all together, this solution is fine by me considering that in > most cases EM is used for extending size without specifying exact size, but > inheriting current one. > > We can also remove this size and calculate everything considering default 16px > accordingly changing all font-sizes accordingly. But in general that would apply > to also margins and all sizes to make it accessible and responsive. > > Not to complicate things we can launch with this solution, if we will have such > requirement for improving accessibility and responsiveness we can update this > accordingly later on. Acknowledged. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:23: color: #3E4347; On 2017/07/31 15:50:43, saroyanm wrote: > According to the style guide, I guess this should be "#4A4A4A" Done. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:70: color: #0A9BD0; On 2017/07/31 15:50:42, saroyanm wrote: > I think this should be #5CBCE1 according to the style guide. The style guide states that text links should have the color `#099CD0`. The color palette only includes the color `#099DD1` though so I'll take that one instead. I also pointed those differing colors out to Jeen. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:98: border: 1px solid #BCBCBC; On 2017/07/31 15:50:42, saroyanm wrote: > Newly added language in style guide, has also different border. > While the style guide is not specified in mobile spec. I'll leave it up to you > if you want to implement it or not, but if you will skip it should be fine for > the launch IMO. This feature doesn't exist in the mobile options page so I'll leave it out. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:98: border: 1px solid #BCBCBC; On 2017/07/31 15:50:42, saroyanm wrote: > I think the border color should be "#BBBBBB" according to the style guide. Done. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:105: flex: 1; On 2017/07/31 15:50:41, saroyanm wrote: > I have assumption that you want to fill whole free space, I assume you want here > flex: auto; ? > As in example here -> https://developer.mozilla.org/en/docs/Web/CSS/flex All those seem to be equivalent: - `flex: auto` - `flex: 1` - `flex-grow: 1` According to the article "For most purposes, authors should set flex to one of the following values: auto, initial, none, or a positive unitless number." Therefore each of the above variants should be fine so I changed it to "auto" as requested to keep things a bit more consistent. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:118: border-bottom: 1px solid #BCBCBC; On 2017/07/31 15:50:43, saroyanm wrote: > The appearance of this element is different in desktop and mobile, I think make > sense to follow the image provided in the Mobile version. So I think we just > need to adapt the color which is #BBB I think. Done. Yeah, I agree that we should prioritize the styles that are defined in the mobile spec and only take the desktop ones as guidelines or ignore them where they don't make sense. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:123: color: #9E9E9E; On 2017/07/31 15:50:41, saroyanm wrote: > According to Style guide, this is #4A4A4A. It looks like the placeholder text should be `#BBBBBB` looking at page 2 and page 9 of the style guide. It'd also be more similar to this color. Also, I noticed that Firefox applies some transparency to placeholders so I fixed that too. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:144: flex-grow: 1; On 2017/07/31 15:50:42, saroyanm wrote: > I'll suggest to use "flex" instead as it's done for "ul > li > span", for > consistency reasons. Done. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:155: flex-shrink: 0; On 2017/07/31 15:50:43, saroyanm wrote: > Not sure if this is required. We don't want the image to shrink below its regular width or otherwise it'll be cut off. Alternatively, we could set `min-width` and `max-width` but this appears to be more appropriate. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:158: background-image: url(mobile/toggle.png); On 2017/07/31 15:50:41, saroyanm wrote: > Detail: while we are using SVGs for other icons, I think it would be consistent > to use SVGs for this as well, but I think you used PNG here as it's more > convenient for switching the state. I can see pros and cons of both approach > even I lean towards using SVG I'll leave it up to you to decide if you think > it's better to use PNG here as well. This is what I got from Jeen so I just used it. I guess I could easily replace it with an SVG file so if you want you can check back with her about whether you two prefer using SVGs instead. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:169: height: 45px; On 2017/07/31 15:50:40, saroyanm wrote: > Suggestion: let's use padding not to be dependent on font-size. I get what you mean but this is a bit tricky in this case because the height of the button should be equal to the height of a list item. The height of a list item, however, is not necessarily text height plus vertical padding because the icon inside the list item has a fixed height of `36px`. Therefore this height needs to be `36px + 5px * 2` so I changed this to `46px` and added a comment to reflect this accordingly. However, I made the suggested change to the height of the primary and secondary buttons since this relation doesn't apply to them. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:173: color: #0A9BD0; On 2017/07/31 15:50:41, saroyanm wrote: > I think this should be "#099CD0" according to the styleguide. Done. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:189: border-color: #0A9BD0; On 2017/07/31 15:50:43, saroyanm wrote: > "#5CBCE1" -styleguide. On page 8 of the style guide it says that we should use `#0A9DD1` for the primary button's active and `#5CBCE1` for its inactive state. Since there is no `#0A9DD1` in the color palette I used `#099DD1` instead again. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:195: border-color: #BCBCBC; On 2017/07/31 15:50:41, saroyanm wrote: > "#0A9DD1" - styleguide. That's not what's shown in https://bytebucket.org/adblockplus/spec/raw/395c6193eb32aeaeb086cf90d792b83d2... though so maybe best to stick to the mobile design here since it's more specific. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:203: background-color: #0A9BD0; On 2017/07/31 15:50:40, saroyanm wrote: > Suggestion "#5CBCE1" for consistency, if the color is not set in SVG. Done. Used `#099DD1` instead again because it's most similar to a link. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:207: ul + button Note that I noticed that the content of this button should be left-aligned so I had to change this rule. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:210: border: 1px solid #BCBCBC; On 2017/07/31 15:50:41, saroyanm wrote: > #BBB Done. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:232: max-width: 25em; On 2017/07/31 15:50:41, saroyanm wrote: > Why are we using EMs here ? Other widths are specified in PXs. Because it's specifically mentioned in the spec: "On larger screens, the modal dialog should maintain its fixed height and extend to a maximum width of 25em." https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:234: border: 1px solid #BCBCBC; On 2017/07/31 15:50:42, saroyanm wrote: > #BBB Done. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:268: border-top: 1px solid #BCBCBC; On 2017/07/31 15:50:40, saroyanm wrote: > #BBB Done. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:281: color: #D1142A; On 2017/07/31 15:50:43, saroyanm wrote: > #C11D26 (Note: The visuals are different, but this one stands for RED color) Done. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:294: border-color: #D1142A; On 2017/07/31 15:50:43, saroyanm wrote: > #C11D26 Done. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:309: max-height: calc(100vh - 40px); On 2017/07/31 15:50:41, saroyanm wrote: > Why Calc is marked as experimental still ? > https://developer.mozilla.org/en-US/docs/Web/CSS/calc > I thought it's in CSS3 ? > > Alternatively we can use percentage ? Seems likethe CSS Values and Module Level 3 standard is still in its Candidate Recommendation stage (see https://www.w3.org/TR/css3-values/#calc-notation) but it's neither prefixed nor behind a flag anymore so I'd assume that it should be safe for use. Furthermore, there are also no warning signs on http://caniuse.com/#search=calc. Usually, we should avoid using `calc()` whenever possible anyway because it's not very performant so I looked into other ways to achieve the same effect and accomplished to do it with flexbox. https://codereview.adblockplus.org/29488575/diff/29491653/skin/mobile-options... skin/mobile-options.css:326: color: #A1A1A1; On 2017/07/31 15:50:42, saroyanm wrote: > This is not specified, maybe: #BBB Done.
What do you think about implementing the mobile UI and applying the style guide changes to it separately afterwards? Because the guide is still under development and we've shifted our focus to the desktop version now.
Sorry Thomas that it took so long for me to get back to this review, I think we are close. I haven't review the Style as I do agree with you and I think it's fine to lunch something even the styles will not be perfect, if needed we can always adjust styles. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.html File mobile-options.html (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.html... mobile-options.html:67: <button class="i18n_mops_filterlist_add_url" Button inside of menu has not been implemented yet according to -> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/menu#Menu_button https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:23: const {getMessage} = ext.i18n; Shouldn't the "ext", be declared as global ? The eslint doesn't fire an error, but I did noticed that on regular options page as well. Am I missing something here ? https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:58: if (onclick) Considering the fact that we have "onclick" function specified in file scope this will always be true. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:137: get("#acceptableAds").checked = true; Detail: this ID is used couple of times, if you will decide to fix the "detail" of "#subscriptions-recommended" I think make sense to fix this as well. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:157: let recommended = get(`#subscriptions-recommended [data-url="${url}"]`); Detail: the "#subscriptions-recommended" ID is used in multiple places, I think you can use that in the file scope and use "Origin" to select recommended using the "get()" method. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:201: field.value = (options && field.name in options) ? options[field.name] : ""; Detail: this exceeds the characters limit. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:229: create(listRecommended, "li", title, {"data-url": url}, Detail: I think we also should specify role="button" for this list items https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:282: if (errors.length < 1) This doesn't pass ESlint indentation chekck for some reason. There are some more errors similar to this, but this error doesn't make sense for me. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:399: // We're also receiving these messages for subscriptions that are not Detail: exceeding 80 chars.
https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:58: if (onclick) On 2017/08/27 17:44:02, saroyanm wrote: > Considering the fact that we have "onclick" function specified in file scope > this will always be true. Nevermind it will not, local scope will override the value anyway..
New patch set is up and I rebased it to master. For that I had to rewrite the data structures for the subscription initialization in background.js to accomodate the changes that were made for the General tab. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.html File mobile-options.html (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.html... mobile-options.html:67: <button class="i18n_mops_filterlist_add_url" On 2017/08/27 17:44:00, saroyanm wrote: > Button inside of menu has not been implemented yet according to -> > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/menu#Menu_button This is not what's mentioned in the link you refer to. It's about a dropdown button (i.e. "a button, which displays a menu when clicked") whereas here we have a button inside a menu. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:23: const {getMessage} = ext.i18n; On 2017/08/27 17:44:01, saroyanm wrote: > Shouldn't the "ext", be declared as global ? > The eslint doesn't fire an error, but I did noticed that on regular options page > as well. > > Am I missing something here ? We define `ext` as a global in .eslintrc.json so no need to define it here. Note that I did get some ESLint errors about "max-len" here and "indent" in background.js as well as messageResponder.js so I fixed those since I'm already touching those as part of this change. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:137: get("#acceptableAds").checked = true; On 2017/08/27 17:44:01, saroyanm wrote: > Detail: this ID is used couple of times, if you will decide to fix the "detail" > of "#subscriptions-recommended" I think make sense to fix this as well. See other comment for the reply. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:157: let recommended = get(`#subscriptions-recommended [data-url="${url}"]`); On 2017/08/27 17:44:01, saroyanm wrote: > Detail: the "#subscriptions-recommended" ID is used in multiple places, I think > you can use that in the file scope and use "Origin" to select recommended using > the "get()" method. Not sure what the underlying argument is. - If the issue is the duplication of the ID name, I could make it a constant for this file. - If the issue is the duplication of calls to retrieve the DOM element, we usually look at that on a function-level so that we don't keep references to DOM nodes around for too long. - If the issue is the performance impact on retrieving the DOM element each time, I'd consider this to be premature optimization. - If the issue is none of the above, please let me know. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:201: field.value = (options && field.name in options) ? options[field.name] : ""; On 2017/08/27 17:44:01, saroyanm wrote: > Detail: this exceeds the characters limit. Done. As mentioned, I also fixed linter errors in the other files. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:229: create(listRecommended, "li", title, {"data-url": url}, On 2017/08/27 17:44:00, saroyanm wrote: > Detail: I think we also should specify role="button" for this list items We don't include Accessibility in the mobile options page spec which is why no work has been done in that regard. Due to that but also the fact that this page is going to be replaced with the regular options page which will introduced Accessibility support, I'd recommend not to spend time on such changes. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:282: if (errors.length < 1) On 2017/08/27 17:44:02, saroyanm wrote: > This doesn't pass ESlint indentation chekck for some reason. > There are some more errors similar to this, but this error doesn't make sense > for me. I'm not getting an error here. What rule does it mention? https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:399: // We're also receiving these messages for subscriptions that are not On 2017/08/27 17:44:01, saroyanm wrote: > Detail: exceeding 80 chars. Done.
https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.html File mobile-options.html (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.html... mobile-options.html:67: <button class="i18n_mops_filterlist_add_url" On 2017/08/28 12:04:20, Thomas Greiner wrote: > On 2017/08/27 17:44:00, saroyanm wrote: > > Button inside of menu has not been implemented yet according to -> > > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/menu#Menu_button > > This is not what's mentioned in the link you refer to. It's about a dropdown > button (i.e. "a button, which displays a menu when clicked") whereas here we > have a button inside a menu. Right, I found that link after this failed the HTML validation -> https://validator.w3.org But apparently the link was wrong. The error states: "Element button not allowed as child of element menu in this context" https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:23: const {getMessage} = ext.i18n; On 2017/08/28 12:04:21, Thomas Greiner wrote: > On 2017/08/27 17:44:01, saroyanm wrote: > > Shouldn't the "ext", be declared as global ? > > The eslint doesn't fire an error, but I did noticed that on regular options > page > > as well. > > > > Am I missing something here ? > > We define `ext` as a global in .eslintrc.json so no need to define it here. > > Note that I did get some ESLint errors about "max-len" here and "indent" in > background.js as well as messageResponder.js so I fixed those since I'm already > touching those as part of this change. Acknowledged. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:157: let recommended = get(`#subscriptions-recommended [data-url="${url}"]`); On 2017/08/28 12:04:20, Thomas Greiner wrote: > On 2017/08/27 17:44:01, saroyanm wrote: > > Detail: the "#subscriptions-recommended" ID is used in multiple places, I > think > > you can use that in the file scope and use "Origin" to select recommended > using > > the "get()" method. > > Not sure what the underlying argument is. > - If the issue is the duplication of the ID name, I could make it a constant for > this file. Yes I was referring to this . > - If the issue is the duplication of calls to retrieve the DOM element, we > usually look at that on a function-level so that we don't keep references to DOM > nodes around for too long. > - If the issue is the performance impact on retrieving the DOM element each > time, I'd consider this to be premature optimization. > - If the issue is none of the above, please let me know. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:229: create(listRecommended, "li", title, {"data-url": url}, On 2017/08/28 12:04:22, Thomas Greiner wrote: > On 2017/08/27 17:44:00, saroyanm wrote: > > Detail: I think we also should specify role="button" for this list items > > We don't include Accessibility in the mobile options page spec which is why no > work has been done in that regard. > > Due to that but also the fact that this page is going to be replaced with the > regular options page which will introduced Accessibility support, I'd recommend > not to spend time on such changes. Acknowledged. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:282: if (errors.length < 1) On 2017/08/28 12:04:21, Thomas Greiner wrote: > On 2017/08/27 17:44:02, saroyanm wrote: > > This doesn't pass ESlint indentation chekck for some reason. > > There are some more errors similar to this, but this error doesn't make sense > > for me. > > I'm not getting an error here. What rule does it mention? Indent: "282:11 error Expected indentation of 8 spaces but found 10 indent"
https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.html File mobile-options.html (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.html... mobile-options.html:67: <button class="i18n_mops_filterlist_add_url" On 2017/08/28 12:27:19, saroyanm wrote: > On 2017/08/28 12:04:20, Thomas Greiner wrote: > > On 2017/08/27 17:44:00, saroyanm wrote: > > > Button inside of menu has not been implemented yet according to -> > > > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/menu#Menu_button > > > > This is not what's mentioned in the link you refer to. It's about a dropdown > > button (i.e. "a button, which displays a menu when clicked") whereas here we > > have a button inside a menu. > > Right, I found that link after this failed the HTML validation -> > https://validator.w3.org But apparently the link was wrong. > > The error states: "Element button not allowed as child of element menu in this > context" Ok, that makes sense. Sounds similar to putting `<button>` into a `<ul>` element so I agree that we should avoid doing this. Done. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:157: let recommended = get(`#subscriptions-recommended [data-url="${url}"]`); On 2017/08/28 12:27:19, saroyanm wrote: > On 2017/08/28 12:04:20, Thomas Greiner wrote: > > On 2017/08/27 17:44:01, saroyanm wrote: > > > Detail: the "#subscriptions-recommended" ID is used in multiple places, I > > think > > > you can use that in the file scope and use "Origin" to select recommended > > using > > > the "get()" method. > > > > Not sure what the underlying argument is. > > - If the issue is the duplication of the ID name, I could make it a constant > for > > this file. > Yes I was referring to this . > > - If the issue is the duplication of calls to retrieve the DOM element, we > > usually look at that on a function-level so that we don't keep references to > DOM > > nodes around for too long. > > - If the issue is the performance impact on retrieving the DOM element each > > time, I'd consider this to be premature optimization. > > - If the issue is none of the above, please let me know. > Acknowledged. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:282: if (errors.length < 1) On 2017/08/28 12:27:19, saroyanm wrote: > On 2017/08/28 12:04:21, Thomas Greiner wrote: > > On 2017/08/27 17:44:02, saroyanm wrote: > > > This doesn't pass ESlint indentation chekck for some reason. > > > There are some more errors similar to this, but this error doesn't make > sense > > > for me. > > > > I'm not getting an error here. What rule does it mention? > > Indent: "282:11 error Expected indentation of 8 spaces but found 10 indent" Done. Seems like we updated eslint-config-eyeo. At least I'm seeing it as "indent-legacy" (see https://hg.adblockplus.org/codingtools/rev/916665587cad) after upgrading to the latest version.
LGTM with a detail and considering the plan of finalizing design implementation/review separately, when it's ready. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:157: let recommended = get(`#subscriptions-recommended [data-url="${url}"]`); On 2017/08/28 13:14:05, Thomas Greiner wrote: > On 2017/08/28 12:27:19, saroyanm wrote: > > On 2017/08/28 12:04:20, Thomas Greiner wrote: > > > On 2017/08/27 17:44:01, saroyanm wrote: > > > > Detail: the "#subscriptions-recommended" ID is used in multiple places, I > > > think > > > > you can use that in the file scope and use "Origin" to select recommended > > > using > > > > the "get()" method. > > > > > > Not sure what the underlying argument is. > > > - If the issue is the duplication of the ID name, I could make it a constant > > for > > > this file. > > Yes I was referring to this . > > > - If the issue is the duplication of calls to retrieve the DOM element, we > > > usually look at that on a function-level so that we don't keep references to > > DOM > > > nodes around for too long. > > > - If the issue is the performance impact on retrieving the DOM element each > > > time, I'd consider this to be premature optimization. > > > - If the issue is none of the above, please let me know. > > > > Acknowledged. Detail: this change didn't land, but it's trivial.
Thanks for the quick review. Please let me know if you want me to elaborate on what I wrote in my comment. Otherwise I'd go on and close the review. https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:157: let recommended = get(`#subscriptions-recommended [data-url="${url}"]`); On 2017/08/28 13:52:05, saroyanm wrote: > On 2017/08/28 13:14:05, Thomas Greiner wrote: > > On 2017/08/28 12:27:19, saroyanm wrote: > > > On 2017/08/28 12:04:20, Thomas Greiner wrote: > > > > On 2017/08/27 17:44:01, saroyanm wrote: > > > > > Detail: the "#subscriptions-recommended" ID is used in multiple places, > I > > > > think > > > > > you can use that in the file scope and use "Origin" to select > recommended > > > > using > > > > > the "get()" method. > > > > > > > > Not sure what the underlying argument is. > > > > - If the issue is the duplication of the ID name, I could make it a > constant > > > for > > > > this file. > > > Yes I was referring to this . > > > > - If the issue is the duplication of calls to retrieve the DOM element, we > > > > usually look at that on a function-level so that we don't keep references > to > > > DOM > > > > nodes around for too long. > > > > - If the issue is the performance impact on retrieving the DOM element > each > > > > time, I'd consider this to be premature optimization. > > > > - If the issue is none of the above, please let me know. > > > > > > > Acknowledged. > > Detail: this change didn't land, but it's trivial. As I mentioned, throughout all of our code "we don't keep references to DOM nodes around for too long" so it sounded like you didn't have anything to add to that.
https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:157: let recommended = get(`#subscriptions-recommended [data-url="${url}"]`); On 2017/08/28 14:00:12, Thomas Greiner wrote: > On 2017/08/28 13:52:05, saroyanm wrote: > > On 2017/08/28 13:14:05, Thomas Greiner wrote: > > > On 2017/08/28 12:27:19, saroyanm wrote: > > > > On 2017/08/28 12:04:20, Thomas Greiner wrote: > > > > > On 2017/08/27 17:44:01, saroyanm wrote: > > > > > > Detail: the "#subscriptions-recommended" ID is used in multiple > places, > > I > > > > > think > > > > > > you can use that in the file scope and use "Origin" to select > > recommended > > > > > using > > > > > > the "get()" method. > > > > > > > > > > Not sure what the underlying argument is. > > > > > - If the issue is the duplication of the ID name, I could make it a > > constant > > > > for > > > > > this file. > > > > Yes I was referring to this . > > > > > - If the issue is the duplication of calls to retrieve the DOM element, > we > > > > > usually look at that on a function-level so that we don't keep > references > > to > > > > DOM > > > > > nodes around for too long. > > > > > - If the issue is the performance impact on retrieving the DOM element > > each > > > > > time, I'd consider this to be premature optimization. > > > > > - If the issue is none of the above, please let me know. > > > > > > > > > > Acknowledged. > > > > Detail: this change didn't land, but it's trivial. > > As I mentioned, throughout all of our code "we don't keep references to DOM > nodes around for too long" so it sounded like you didn't have anything to add to > that. I did commented under the line: "If the issue is the duplication of the ID name, I could make it a constant for this file." But as I mentioned it's trivial. So feel free either creating constants for duplicated IDs, or push as it is right now.
https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js File mobile-options.js (right): https://codereview.adblockplus.org/29488575/diff/29502627/mobile-options.js#n... mobile-options.js:157: let recommended = get(`#subscriptions-recommended [data-url="${url}"]`); On 2017/08/28 14:05:24, saroyanm wrote: > On 2017/08/28 14:00:12, Thomas Greiner wrote: > > On 2017/08/28 13:52:05, saroyanm wrote: > > > On 2017/08/28 13:14:05, Thomas Greiner wrote: > > > > On 2017/08/28 12:27:19, saroyanm wrote: > > > > > On 2017/08/28 12:04:20, Thomas Greiner wrote: > > > > > > On 2017/08/27 17:44:01, saroyanm wrote: > > > > > > > Detail: the "#subscriptions-recommended" ID is used in multiple > > places, > > > I > > > > > > think > > > > > > > you can use that in the file scope and use "Origin" to select > > > recommended > > > > > > using > > > > > > > the "get()" method. > > > > > > > > > > > > Not sure what the underlying argument is. > > > > > > - If the issue is the duplication of the ID name, I could make it a > > > constant > > > > > for > > > > > > this file. > > > > > Yes I was referring to this . > > > > > > - If the issue is the duplication of calls to retrieve the DOM > element, > > we > > > > > > usually look at that on a function-level so that we don't keep > > references > > > to > > > > > DOM > > > > > > nodes around for too long. > > > > > > - If the issue is the performance impact on retrieving the DOM element > > > each > > > > > > time, I'd consider this to be premature optimization. > > > > > > - If the issue is none of the above, please let me know. > > > > > > > > > > > > > Acknowledged. > > > > > > Detail: this change didn't land, but it's trivial. > > > > As I mentioned, throughout all of our code "we don't keep references to DOM > > nodes around for too long" so it sounded like you didn't have anything to add > to > > that. > > I did commented under the line: > "If the issue is the duplication of the ID name, I could make it a constant for > this file." > > But as I mentioned it's trivial. So feel free either creating constants for > duplicated IDs, or push as it is right now. Ok, I only looked at the text below your message. I uploaded the patch set with the change but I'm not sure whether it's an improvement. I don't mind it either way so feel free to decide yourself whether or not you want to keep this change or whether you think it's better not to make this change.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
