Issue 29333819: Issue 2375 - Implement "Blocking lists" section in new options page (Closed)
|
Created:
Jan. 18, 2016, 9:50 a.m. by saroyanm Modified:
Feb. 5, 2016, 11:34 a.m. Reviewers:
Thomas Greiner CC:
Felix Dahlke Visibility:
Public. |
DescriptionIssue 2375 - Implement "Blocking lists" section in new options page
Patch Set 1 #
Total comments: 112
Patch Set 2 : Addressed Thomas comments #
Total comments: 56
Patch Set 3 : #
Total comments: 24
Patch Set 4 : Rebase to d12b18c2a168 #
Total comments: 1
Patch Set 5 : #Patch Set 6 : #
Total comments: 30
Patch Set 7 : Small fixes and fix for Object.defineProperty #
Total comments: 6
Patch Set 8 : Small fixes and improved download messages for filter lists #
Total comments: 10
Patch Set 9 : Fixed the progress indicator and small fixes #
Total comments: 2
MessagesTotal messages: 18
Finally, @Thomas can you please have a look.
https://codereview.adblockplus.org/29333819/diff/29333820/background.js File background.js (right): https://codereview.adblockplus.org/29333819/diff/29333820/background.js#newco... background.js:195: modules.filterNotifier.FilterNotifier.triggerListeners("subscription.updated", subscription); Since you added error messages as part of this review, what about adding the ability to simulate those in the test environment? https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:162: "options_tableCol_description": { There's no "description" column anymore so please remove this message. https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:168: "message": "Date" According to the style guide this should be "Last update". https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:176: "message": "Update all blocking lists" According to the style guide this should be lower-case. https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:192: "message": "Failed, not a valid filters list" Typo: Replace "filters list" with "filter list". https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:198: "options_blockingList_update_now": { Detail: The naming is a bit inconsistent because shouldn't it be either "options_blockingList_updateNow" or "options_blockingList_update"? https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:215: "description": "Last subscription name in blocking list section in advnaced tab", Typo: Replace "advnaced" with "advanced". https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:215: "description": "Last subscription name in blocking list section in advnaced tab", Detail: Please don't mention details that depend on how the UI looks because that might change. This text should describe the purpose of this text rather than how it will look like. e.g. Name of custom subscription entry in blocking list section in Advanced tab. https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:220: "message": "edit your blocking list" I thought we decided on calling them "filter lists"? Maybe quickly check back with Lisa to ensure that each of those texts is up-to-date. https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js... messageResponder.js:50: "downloadStatus", "lastDownload", "homepage", "lastSuccess", "title", "url"]); Detail: What about keeping it alphabetically ordered and considering the 80 character line limitation? https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js... messageResponder.js:50: "downloadStatus", "lastDownload", "homepage", "lastSuccess", "title", "url"]); Do we want "lastDownload", "lastSuccess" or both? https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js... messageResponder.js:357: case "subscriptions.toggleState": I'd rather add this to "subscriptions.toggle" to avoid duplication. You can introduce the flag "keepInstalled" to indicate whether it should be added/removed or merely en-/disabled. https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js... messageResponder.js:366: case "subscriptions.updateAll": Why not add this to "subscriptions.update"? If `message.url` is not given we could update all. https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js... messageResponder.js:379: case "subscriptions.website": This belongs into "app.open". However, I'd suggest going with regular hyperlinks for the subscription homepage and source. Or is there a need for opening them this way? https://codereview.adblockplus.org/29333819/diff/29333820/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcode32 options.html:32: <body data-tab="advanced,filter-lists"> I suppose you added this for testing. Please revert it, if that's the case. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcode46 options.html:46: <li id="tab-advanced" data-action="switch-tab" data-tab="advanced,filter-lists"> Detail: "abc,def" looks like a list of elements but it seems that you want to indicate that on page "abc" it should navigate to "def". Therefore I'd suggest to use either "abc/def" or "abc.def" to indicate that hierarchy. Alternatively, you could rewrite it as `data-tab="advanced-filterLists"` to use the `li[data-tab|="filterLists"]` selector (see https://www.w3.org/TR/css3-selectors/#attribute-selectors). However, since that wouldn't be a hierarchical notation, I'd advise against it. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:239: <li class="head static"> The headline should semantically not be part of the list. It's meta data and not a list entry. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:248: <a href="#" data-action="open-context-menu" class="arrow"></a> I noticed a bunch of positioning styles in "options.css". Presumably, that's because the DIV below is not directly linked to this anchor tag. Therefore I'd suggest moving the DIV inside the anchor tag to simplify the styling code. Maybe you could even get rid of the DIV around those two elements. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:249: <div role="context-menu"> This role value doesn't exist. For now, it should be sufficient to use a class. If we want to optimize for accessibility, the structure needs to look like that: <ul role="menu" aria-hidden="true"> <li role="menuitem">...</li> ... </ul> However, there may be other things we need to consider as well (e.g. tab index) so might be best to tackle that in a separate issue. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:251: <a class="update-now" data-action="update-now" href="#"></a> Detail: Please remove hyperlinking to "#" for links that don't link anywhere. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:261: </template> This causes the selector `.table.cols li:nth-child(even)` to break the zebra pattern. Instead you could change the selector to `.table.cols li:nth-of-type(even)`. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:263: <input type="checkbox" id="own-list" checked="true" disabled="true" /> This checkbox doesn't do anything so either implement the en-/disabling of this custom subscription or visually indicate that this checkbox is disabled. https://codereview.adblockplus.org/29333819/diff/29333820/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode106 options.js:106: var access = (item.url || item.text).replace(/'/g, "\\'"); I notice the issues with accessing elements by their index so I understand the need for using this accessor. However, please extract this into its own function so that we don't have to implement this logic wherever we need to determine the access value of an item. For example: Collection.prototype._createElementQuery = function(item) { var access = (item.url || item.text).replace(/'/g, "\\'"); return function(container) { return container.querySelector("[data-access='" + access + "']"); }; }; https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode124 options.js:124: var staticElements = []; This logic is quite complicated. Assuming that you merely want to know at which position elements should be inserted you could go with the following approach: 1. Create a document fragment: document.createDocumentFragment() 2. Append list elements to the document fragment: fragment.appendChild() 3. Insert the document fragment at the appropriate position: template.parentNode.insertBefore(fragment, template) Obviously, that means that you won't be able to clear it with `table.innerHTML = ""` anymore and that you therefore have to remove each element individually but that should be fine since we're only calling this method in `populateLists()`. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode142 options.js:142: Collection.prototype.getTableIds = function() This information is an implementation detail and should therefore stay within the Collection. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode175 options.js:175: var timedate = i18n_timeDateStrings(subscription.lastDownload * 1000); This function returns the localized date-time string which is not what you're looking for because it will differ (e.g. "1970/30/1" or "1970/1/30" or "1.1.1970") and thereby cause unexpected results and forces you to do all those modificiations and checks below. Therefore please don't use this function and instead implement it yourself. Feel free to add it to "i18n.js" but note that we cannot remove "i18n_timeDateStrings()" yet because it's still being used in the current options page. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode197 options.js:197: function onToggleSubscriptionClick(e) I'd suggest renaming this function to "toggleRemoveSubscription" and the one below to "toggleDisableSubscription" to clarify how their behavior differs. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode215 options.js:215: var subscriptionUrl = e.target.parentNode.getAttribute("data-access"); What about reusing `getParentAccessElement()` since you already created it? https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode489 options.js:489: function getParentAccessElement() Please avoid declaring functions inside other functions or otherwise they're being created everytime when the function is called. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode492 options.js:492: while (!elementCopy.dataset.access) We can't use `Element.dataset` due to older Safari versions so please use `Element.getAttribute()`. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode557 options.js:557: case "open-context-menu": We should improve this mechanism because it requires quite a bit of extra work to get the UX right. My suggestion would be to listen to clicks: - click on subscription title: - if context menu hidden: show context menu - else: hide context menu - click outside of context menu: hide context menu - click on action item in context menu: hide context menu - click somewhere else in context menu: ignore That would also be consistent with other context menus (e.g. right-click context menu). https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode611 options.js:611: function updateTemplate(template, selector, messageId) This functionality belongs into "i18n.js". If elements inside templates are not retrieved on selecting `[class^="i18n_"]` we need to add that it should also search for those within `<template>` elements. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode916 options.js:916: updateSubscription(subscription); That's not what "updated" is meant to notify you of. In FilterNotifier, "subscription.updated" is triggered whenever filters in the subscription change. Modifications to the subscription itself have their own events: - disabled - downloadStatus - homepage - lastDownload - title https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:187: body[data-tab*="advanced"] #tab-advanced, We don't expect this to be positioned anywhere inside "data-tab" so `data-tab^="advanced"` should be sufficient. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:378: } You can merge those two rules above. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:668: #filter-lists .table li According to the style guide the padding above and below the text should differ for `li.head` https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:668: #filter-lists .table li I noticed that long filter list titles cause the line's height to expand. Please check back with Sven whether that is intended or whether (a) the title should be cut off or (b) the table's width should increase. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:691: #filter-lists .controls Detail: The margins for the buttons are not according to the style guide. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:696: #filter-lists .arrow I understand that we might not want to reorder the existing styles in this file but at least with the ones we add we should be consistent with the CSS coding style regarding the ordering of properties. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:706: margin: 0px; You're overriding the margin you've set above so it's not being applied. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:709: #filter-lists .table li:last-of-type a Detail: According to the style guide this element should be aligned to the right. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:888: border-bottom:none; Detail: Missing whitespace after colon. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:992: left: calc(-100%/2); That's the same as writing `left: -50%`. Anyway, due to the `-4px` offset of the arrow, it needs to be written as `left: calc(-50% + 2px)` to center it properly. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:994: padding: 10px; According to the style guide each menu item should have the same height. However, due to this padding the first and last items' height is too big. Therefore this needs to be changed to `padding: 0px 10px`. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:1028: width: 14px; Why does each of those icons have its own width and height? Please make sure that they have the same dimensions so that we can add, remove or replace them easily. Note that this is also in-line with the style guide which specifies to use 16x16.
https://codereview.adblockplus.org/29333819/diff/29333820/background.js File background.js (right): https://codereview.adblockplus.org/29333819/diff/29333820/background.js#newco... background.js:195: modules.filterNotifier.FilterNotifier.triggerListeners("subscription.updated", subscription); On 2016/01/19 11:27:26, Thomas Greiner wrote: > Since you added error messages as part of this review, what about adding the > ability to simulate those in the test environment? Done. https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:162: "options_tableCol_description": { On 2016/01/19 11:27:27, Thomas Greiner wrote: > There's no "description" column anymore so please remove this message. Done. https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:168: "message": "Date" On 2016/01/19 11:27:26, Thomas Greiner wrote: > According to the style guide this should be "Last update". Done. https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:176: "message": "Update all blocking lists" On 2016/01/19 11:27:27, Thomas Greiner wrote: > According to the style guide this should be lower-case. Done. https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:192: "message": "Failed, not a valid filters list" On 2016/01/19 11:27:26, Thomas Greiner wrote: > Typo: Replace "filters list" with "filter list". Done. https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:198: "options_blockingList_update_now": { On 2016/01/19 11:27:26, Thomas Greiner wrote: > Detail: The naming is a bit inconsistent because shouldn't it be either > "options_blockingList_updateNow" or "options_blockingList_update"? Done. https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:215: "description": "Last subscription name in blocking list section in advnaced tab", On 2016/01/19 11:27:26, Thomas Greiner wrote: > Detail: Please don't mention details that depend on how the UI looks because > that might change. This text should describe the purpose of this text rather > than how it will look like. > > e.g. Name of custom subscription entry in blocking list section in Advanced tab. Done. https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:215: "description": "Last subscription name in blocking list section in advnaced tab", On 2016/01/19 11:27:27, Thomas Greiner wrote: > Typo: Replace "advnaced" with "advanced". Done. https://codereview.adblockplus.org/29333819/diff/29333820/locale/en-US/option... locale/en-US/options.json:220: "message": "edit your blocking list" On 2016/01/19 11:27:27, Thomas Greiner wrote: > I thought we decided on calling them "filter lists"? Maybe quickly check back > with Lisa to ensure that each of those texts is up-to-date. Done. https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js... messageResponder.js:50: "downloadStatus", "lastDownload", "homepage", "lastSuccess", "title", "url"]); On 2016/01/19 11:27:27, Thomas Greiner wrote: > Do we want "lastDownload", "lastSuccess" or both? In current option page, seems like we are using lastDownload, so I'll keep lastDownload for now, in case we will need lastSuccess in future we can always add. https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js... messageResponder.js:50: "downloadStatus", "lastDownload", "homepage", "lastSuccess", "title", "url"]); On 2016/01/19 11:27:27, Thomas Greiner wrote: > Detail: What about keeping it alphabetically ordered and considering the 80 > character line limitation? Done. https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js... messageResponder.js:357: case "subscriptions.toggleState": On 2016/01/19 11:27:27, Thomas Greiner wrote: > I'd rather add this to "subscriptions.toggle" to avoid duplication. You can > introduce the flag "keepInstalled" to indicate whether it should be > added/removed or merely en-/disabled. Done. https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js... messageResponder.js:366: case "subscriptions.updateAll": On 2016/01/19 11:27:27, Thomas Greiner wrote: > Why not add this to "subscriptions.update"? If `message.url` is not given we > could update all. Done. https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js... messageResponder.js:379: case "subscriptions.website": On 2016/01/19 11:27:27, Thomas Greiner wrote: > This belongs into "app.open". > > However, I'd suggest going with regular hyperlinks for the subscription homepage > and source. Or is there a need for opening them this way? Done. https://codereview.adblockplus.org/29333819/diff/29333820/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcode32 options.html:32: <body data-tab="advanced,filter-lists"> On 2016/01/19 11:27:28, Thomas Greiner wrote: > I suppose you added this for testing. Please revert it, if that's the case. Done. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcode46 options.html:46: <li id="tab-advanced" data-action="switch-tab" data-tab="advanced,filter-lists"> On 2016/01/19 11:27:28, Thomas Greiner wrote: > Detail: "abc,def" looks like a list of elements but it seems that you want to > indicate that on page "abc" it should navigate to "def". Therefore I'd suggest > to use either "abc/def" or "abc.def" to indicate that hierarchy. No, I would like to indicate that the list of tabs should be active after triggering "switch-tab" action, same way we indicate list of actions, when we would like to trigger several actions. But if you think that we need to have hierarchical model of tab management, we can have it, I just can't see benefit. What you think ? https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:239: <li class="head static"> On 2016/01/19 11:27:28, Thomas Greiner wrote: > The headline should semantically not be part of the list. It's meta data and not > a list entry. Done, it's a bit sad, I couldn't find a way how it would be possible to associate the header with column, hope there will be a solution for that case in future without using table. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:248: <a href="#" data-action="open-context-menu" class="arrow"></a> On 2016/01/19 11:27:28, Thomas Greiner wrote: > I noticed a bunch of positioning styles in "options.css". Presumably, that's > because the DIV below is not directly linked to this anchor tag. Therefore I'd > suggest moving the DIV inside the anchor tag to simplify the styling code. > > Maybe you could even get rid of the DIV around those two elements. The problem with double div is that I don't have a fixed width, so I'm possitioning -50% the inner div inside absolute div, so I can have flexible layout. Also having the anchor outside I can change the visibility of sibling div. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:249: <div role="context-menu"> On 2016/01/19 11:27:28, Thomas Greiner wrote: > This role value doesn't exist. For now, it should be sufficient to use a class. > > If we want to optimize for accessibility, the structure needs to look like that: > > <ul role="menu" aria-hidden="true"> > <li role="menuitem">...</li> > ... > </ul> > > However, there may be other things we need to consider as well (e.g. tab index) > so might be best to tackle that in a separate issue. I see, done. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:251: <a class="update-now" data-action="update-now" href="#"></a> On 2016/01/19 11:27:28, Thomas Greiner wrote: > Detail: Please remove hyperlinking to "#" for links that don't link anywhere. Done. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:261: </template> On 2016/01/19 11:27:28, Thomas Greiner wrote: > This causes the selector `.table.cols li:nth-child(even)` to break the zebra > pattern. Instead you could change the selector to `.table.cols > li:nth-of-type(even)`. Done. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:263: <input type="checkbox" id="own-list" checked="true" disabled="true" /> On 2016/01/19 11:27:28, Thomas Greiner wrote: > This checkbox doesn't do anything so either implement the en-/disabling of this > custom subscription or visually indicate that this checkbox is disabled. Not sure if it looks good, if you think it's ugly or doesn't fit to current review I'll implement the toggle. Also make sense to decide whether we need to have context menu for this item or not, anyway I'll suggest to revisit user's filter list item management separately. https://codereview.adblockplus.org/29333819/diff/29333820/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode106 options.js:106: var access = (item.url || item.text).replace(/'/g, "\\'"); On 2016/01/19 11:27:28, Thomas Greiner wrote: > I notice the issues with accessing elements by their index so I understand the > need for using this accessor. However, please extract this into its own function > so that we don't have to implement this logic wherever we need to determine the > access value of an item. > > For example: > > Collection.prototype._createElementQuery = function(item) > { > var access = (item.url || item.text).replace(/'/g, "\\'"); > return function(container) > { > return container.querySelector("[data-access='" + access + "']"); > }; > }; Done, but wouldn't it be simpler to just have a private method that takes two parameters and return the result ? Like: Collection.prototype._getListElement = function(item, container) { return container.querySelector("[data-access='" + (item.url || item.text).replace(/'/g, "\\'") + "']"); }; What is the benefit of returning the function ? Maybe I don't understand your approach. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode124 options.js:124: var staticElements = []; On 2016/01/19 11:27:29, Thomas Greiner wrote: > This logic is quite complicated. Assuming that you merely want to know at which > position elements should be inserted you could go with the following approach: > > 1. Create a document fragment: document.createDocumentFragment() > 2. Append list elements to the document fragment: fragment.appendChild() > 3. Insert the document fragment at the appropriate position: > template.parentNode.insertBefore(fragment, template) > > Obviously, that means that you won't be able to clear it with `table.innerHTML = > ""` anymore and that you therefore have to remove each element individually but > that should be fine since we're only calling this method in `populateLists()`. Currently we are not using createDocumentFragment to add List item, I've updated current removal implementation to only remove the dynamic list items, I also need to remove text nodes otherwise it's becoming messy to always determine the type of the node. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode142 options.js:142: Collection.prototype.getTableIds = function() On 2016/01/19 11:27:29, Thomas Greiner wrote: > This information is an implementation detail and should therefore stay within > the Collection. updated the method to check if the collection has ID. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode175 options.js:175: var timedate = i18n_timeDateStrings(subscription.lastDownload * 1000); On 2016/01/19 11:27:29, Thomas Greiner wrote: > This function returns the localized date-time string which is not what you're > looking for because it will differ (e.g. "1970/30/1" or "1970/1/30" or > "1.1.1970") and thereby cause unexpected results and forces you to do all those > modificiations and checks below. > > Therefore please don't use this function and instead implement it yourself. Feel > free to add it to "i18n.js" but note that we cannot remove > "i18n_timeDateStrings()" yet because it's still being used in the current > options page. I see, I was trying to make it consistent with old code, didn't noticed the localization part thanks, not really sure if we need separate method for this in i18n, because it has nothing to do with localization currently, but I have implemented the logic in current function, I think should be enough for now, but let me know if you expect this logic to do something else. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode197 options.js:197: function onToggleSubscriptionClick(e) On 2016/01/19 11:27:29, Thomas Greiner wrote: > I'd suggest renaming this function to "toggleRemoveSubscription" and the one > below to "toggleDisableSubscription" to clarify how their behavior differs. Done. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode215 options.js:215: var subscriptionUrl = e.target.parentNode.getAttribute("data-access"); On 2016/01/19 11:27:29, Thomas Greiner wrote: > What about reusing `getParentAccessElement()` since you already created it? Done. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode489 options.js:489: function getParentAccessElement() On 2016/01/19 11:27:30, Thomas Greiner wrote: > Please avoid declaring functions inside other functions or otherwise they're > being created everytime when the function is called. Good point. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode492 options.js:492: while (!elementCopy.dataset.access) On 2016/01/19 11:27:29, Thomas Greiner wrote: > We can't use `Element.dataset` due to older Safari versions so please use > `Element.getAttribute()`. Right, done. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode557 options.js:557: case "open-context-menu": On 2016/01/19 11:27:29, Thomas Greiner wrote: > We should improve this mechanism because it requires quite a bit of extra work > to get the UX right. > > My suggestion would be to listen to clicks: > - click on subscription title: > - if context menu hidden: show context menu > - else: hide context menu > - click outside of context menu: hide context menu > - click on action item in context menu: hide context menu > - click somewhere else in context menu: ignore > > That would also be consistent with other context menus (e.g. right-click context > menu). Done, but bit hacky, not sure if it's what you mean. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode611 options.js:611: function updateTemplate(template, selector, messageId) On 2016/01/19 11:27:29, Thomas Greiner wrote: > This functionality belongs into "i18n.js". If elements inside templates are not > retrieved on selecting `[class^="i18n_"]` we need to add that it should also > search for those within `<template>` elements. Done. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode916 options.js:916: updateSubscription(subscription); On 2016/01/19 11:27:29, Thomas Greiner wrote: > That's not what "updated" is meant to notify you of. In FilterNotifier, > "subscription.updated" is triggered whenever filters in the subscription change. > Modifications to the subscription itself have their own events: > - disabled > - downloadStatus > - homepage > - lastDownload > - title Ahh right, thanks for pointing that out. Done. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:187: body[data-tab*="advanced"] #tab-advanced, On 2016/01/19 11:27:31, Thomas Greiner wrote: > We don't expect this to be positioned anywhere inside "data-tab" so > `data-tab^="advanced"` should be sufficient. It depends on other comment regarding showing the tabs in options.html, in case we will have list of tabs that should be active, I think we are good with asterisk selector but if we will indicate only one active tab then this changes are not necessary at all, or if we will consider the order of the selectors then we can go with "^" option. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:378: } On 2016/01/19 11:27:30, Thomas Greiner wrote: > You can merge those two rules above. Done. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:668: #filter-lists .table li On 2016/01/19 11:27:30, Thomas Greiner wrote: > I noticed that long filter list titles cause the line's height to expand. Please > check back with Sven whether that is intended or whether (a) the title should be > cut off or (b) the table's width should increase. Currently it's consistent with other list elements, I'll suggest keep it like this and update that with other list items, because feels like they should behave somehow similar. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:668: #filter-lists .table li On 2016/01/19 11:27:30, Thomas Greiner wrote: > According to the style guide the padding above and below the text should differ > for `li.head` Done. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:691: #filter-lists .controls On 2016/01/19 11:27:31, Thomas Greiner wrote: > Detail: The margins for the buttons are not according to the style guide. Done. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:696: #filter-lists .arrow On 2016/01/19 11:27:30, Thomas Greiner wrote: > I understand that we might not want to reorder the existing styles in this file > but at least with the ones we add we should be consistent with the CSS coding > style regarding the ordering of properties. Done. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:706: margin: 0px; On 2016/01/19 11:27:31, Thomas Greiner wrote: > You're overriding the margin you've set above so it's not being applied. Done. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:709: #filter-lists .table li:last-of-type a On 2016/01/19 11:27:30, Thomas Greiner wrote: > Detail: According to the style guide this element should be aligned to the > right. Done. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:888: border-bottom:none; On 2016/01/19 11:27:31, Thomas Greiner wrote: > Detail: Missing whitespace after colon. Done. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:992: left: calc(-100%/2); On 2016/01/19 11:27:30, Thomas Greiner wrote: > That's the same as writing `left: -50%`. Anyway, due to the `-4px` offset of the > arrow, it needs to be written as `left: calc(-50% + 2px)` to center it properly. Done. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:994: padding: 10px; On 2016/01/19 11:27:30, Thomas Greiner wrote: > According to the style guide each menu item should have the same height. > However, due to this padding the first and last items' height is too big. > Therefore this needs to be changed to `padding: 0px 10px`. Done. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:1028: width: 14px; On 2016/01/19 11:27:31, Thomas Greiner wrote: > Why does each of those icons have its own width and height? Please make sure > that they have the same dimensions so that we can add, remove or replace them > easily. > > Note that this is also in-line with the style guide which specifies to use > 16x16. Done.
https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js... messageResponder.js:50: "downloadStatus", "lastDownload", "homepage", "lastSuccess", "title", "url"]); On 2016/01/22 09:55:06, saroyanm wrote: > On 2016/01/19 11:27:27, Thomas Greiner wrote: > > Do we want "lastDownload", "lastSuccess" or both? > > In current option page, seems like we are using lastDownload, so I'll keep > lastDownload for now, in case we will need lastSuccess in future we can always > add. There's already a ticket requesting for this information to be shown additionally in case of an error (see https://issues.adblockplus.org/ticket/2848) so we can work on it separately. https://codereview.adblockplus.org/29333819/diff/29333820/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcode46 options.html:46: <li id="tab-advanced" data-action="switch-tab" data-tab="advanced,filter-lists"> On 2016/01/22 09:55:07, saroyanm wrote: > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > Detail: "abc,def" looks like a list of elements but it seems that you want to > > indicate that on page "abc" it should navigate to "def". Therefore I'd suggest > > to use either "abc/def" or "abc.def" to indicate that hierarchy. > No, I would like to indicate that the list of tabs should be active after > triggering "switch-tab" action, same way we indicate list of actions, when we > would like to trigger several actions. > But if you think that we need to have hierarchical model of tab management, we > can have it, I just can't see benefit. What you think ? I see what you mean but in this case they are dependent on each other since switching to tab "filter-lists" doesn't do anything unless you also switch to tab "advanced" so in this case "filter-lists" is dependent on "advanced". Note that this is also relevant for deep-linking which I'll introduce in #2409 that allows us to link to a section of the options page that may be in a different tab. Therefore my suggestion would be to use `data-tab="advanced-filterLists"` so that we can link to it via `options.html#advanced-filterLists` and apply styles via `body[data-tab|="advanced"]` and `body[data-tab|="filterLists"]`. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:263: <input type="checkbox" id="own-list" checked="true" disabled="true" /> On 2016/01/22 09:55:08, saroyanm wrote: > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > This checkbox doesn't do anything so either implement the en-/disabling of > this > > custom subscription or visually indicate that this checkbox is disabled. > > Not sure if it looks good, if you think it's ugly or doesn't fit to current > review I'll implement the toggle. > Also make sense to decide whether we need to have context menu for this item or > not, anyway I'll suggest to revisit user's filter list item management > separately. I don't mind to do it in a separate review but we should definitely work on that. https://codereview.adblockplus.org/29333819/diff/29333820/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode106 options.js:106: var access = (item.url || item.text).replace(/'/g, "\\'"); On 2016/01/22 09:55:08, saroyanm wrote: > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > I notice the issues with accessing elements by their index so I understand the > > need for using this accessor. However, please extract this into its own > function > > so that we don't have to implement this logic wherever we need to determine > the > > access value of an item. > > > > For example: > > > > Collection.prototype._createElementQuery = function(item) > > { > > var access = (item.url || item.text).replace(/'/g, "\\'"); > > return function(container) > > { > > return container.querySelector("[data-access='" + access + "']"); > > }; > > }; > > Done, but wouldn't it be simpler to just have a private method that takes two > parameters and return the result ? > Like: > Collection.prototype._getListElement = function(item, container) > { > return container.querySelector("[data-access='" + (item.url || > item.text).replace(/'/g, "\\'") + "']"); > }; > > What is the benefit of returning the function ? > Maybe I don't understand your approach. By returning a function we only create the "access" string once instead of everytime we call the function. It's not a big difference overall but my code was only a suggestion anyway so feel free to implement it either way. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode124 options.js:124: var staticElements = []; On 2016/01/22 09:55:10, saroyanm wrote: > On 2016/01/19 11:27:29, Thomas Greiner wrote: > > This logic is quite complicated. Assuming that you merely want to know at > which > > position elements should be inserted you could go with the following approach: > > > > 1. Create a document fragment: document.createDocumentFragment() > > 2. Append list elements to the document fragment: fragment.appendChild() > > 3. Insert the document fragment at the appropriate position: > > template.parentNode.insertBefore(fragment, template) > > > > Obviously, that means that you won't be able to clear it with `table.innerHTML > = > > ""` anymore and that you therefore have to remove each element individually > but > > that should be fine since we're only calling this method in `populateLists()`. > > Currently we are not using createDocumentFragment to add List item, I've updated > current removal implementation to only remove the dynamic list items, I also > need to remove text nodes otherwise it's becoming messy to always determine the > type of the node. You could use `Element.nextElementSibling` to avoid other kinds of nodes. So checking for the "static" class and `<template>` elements should be sufficient. Alternatively, you could use `table.children` to get a list of all child elements. In any case you need to be careful since you're removing the element from the DOM. The safest approach would be to use `Array.prototype.slice.call(table.children)` and loop over the items in the array. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode175 options.js:175: var timedate = i18n_timeDateStrings(subscription.lastDownload * 1000); On 2016/01/22 09:55:10, saroyanm wrote: > On 2016/01/19 11:27:29, Thomas Greiner wrote: > > This function returns the localized date-time string which is not what you're > > looking for because it will differ (e.g. "1970/30/1" or "1970/1/30" or > > "1.1.1970") and thereby cause unexpected results and forces you to do all > those > > modificiations and checks below. > > > > Therefore please don't use this function and instead implement it yourself. > Feel > > free to add it to "i18n.js" but note that we cannot remove > > "i18n_timeDateStrings()" yet because it's still being used in the current > > options page. > > I see, I was trying to make it consistent with old code, didn't noticed the > localization part thanks, not really sure if we need separate method for this in > i18n, because it has nothing to do with localization currently, but I have > implemented the logic in current function, I think should be enough for now, but > let me know if you expect this logic to do something else. The mere formatting of the string should be generic enough for inclusion in "i18n.js", all the other parts should stay here. e.g. var dateTime = i18n_formatDateTime(date); dateElement.textContent = dateTime[0]; timeElement.textContent = dateTime[1]; Eventually, we might need something even more generic (e.g. `i18n_formatDateTime(date, "YYYY-MM-DD hh:mm")`) but that'd probably be too much effort for this specific use case. Anyway, please note that the code you implemented formats the date as "2016-111-1 1:1" instead of the expected "2016-12-01 01:01". https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode215 options.js:215: var subscriptionUrl = e.target.parentNode.getAttribute("data-access"); On 2016/01/22 09:55:10, saroyanm wrote: > On 2016/01/19 11:27:29, Thomas Greiner wrote: > > What about reusing `getParentAccessElement()` since you already created it? > > Done. I don't see that you changed anything here. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:668: #filter-lists .table li On 2016/01/22 09:55:12, saroyanm wrote: > On 2016/01/19 11:27:30, Thomas Greiner wrote: > > I noticed that long filter list titles cause the line's height to expand. > Please > > check back with Sven whether that is intended or whether (a) the title should > be > > cut off or (b) the table's width should increase. > > Currently it's consistent with other list elements, I'll suggest keep it like > this and update that with other list items, because feels like they should > behave somehow similar. Note that the behavior changes with https://codereview.adblockplus.org/29333262/ but fine with me for now since this change hasn't landed. https://codereview.adblockplus.org/29333819/diff/29334296/background.js File background.js (right): https://codereview.adblockplus.org/29333819/diff/29334296/background.js#newco... background.js:75: this.lastDownload = 1234; Since you've added the setter for "lastDownload" this will trigger the "subscription.lastDownload" event to be triggered whenever a subscription is added which, however, should only happen when a subscription is downloaded. Changing it to `this._lastDownload = 1234;` should be sufficient. https://codereview.adblockplus.org/29333819/diff/29334296/i18n.js File i18n.js (right): https://codereview.adblockplus.org/29333819/diff/29334296/i18n.js#newcode68 i18n.js:68: function addI18nStringsToElements(nodes) Detail: You're calling them "elements" everywhere else so why go with "node" here? https://codereview.adblockplus.org/29333819/diff/29334296/i18n.js#newcode85 i18n.js:85: addI18nStringsToElements(document.querySelectorAll("[class^='i18n_']")); Detail: You're always calling this function using the same selector so to avoid duplication I'd suggest instead only passing the container element and querying for elements inside `addI18nStringsToElements()`. https://codereview.adblockplus.org/29333819/diff/29334296/i18n.js#newcode86 i18n.js:86: var templates = document.querySelectorAll("template"); Detail: Please add a short comment to explain why this is necessary. https://codereview.adblockplus.org/29333819/diff/29334296/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29333819/diff/29334296/locale/en-US/option... locale/en-US/options.json:158: "options_tableCol_name": { Detail: I just noticed that "column" is abbreviated which may be a bit confusing so what about renaming it to "options_column_name"? https://codereview.adblockplus.org/29333819/diff/29334296/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29334296/options.html#newcod... options.html:237: <div id="filter-lists"> Detail: It's unclear what the difference between the IDs "blocking-lists", "filter-lists" and "blocking-lists-table" here so I'd suggest renaming them. e.g. something along the lines of "filterLists" and "filterLists-container" and "filterLists-list" (also since we no longer call them "blocking lists" in the texts) https://codereview.adblockplus.org/29333819/diff/29334296/options.html#newcod... options.html:248: <a href="#" data-action="open-context-menu" class="arrow"></a> Detail: No need to set the "href" attribute here since we're not linking anywhere. https://codereview.adblockplus.org/29333819/diff/29334296/options.html#newcod... options.html:263: <input type="checkbox" id="own-list" checked="true" disabled="true" /> Detail: It doesn't seem you're using this ID here so please remove it. https://codereview.adblockplus.org/29333819/diff/29334296/options.html#newcod... options.html:282: <h2 id="custom-filters-header" class="i18n_options_customFilters_title"></h2> Why are you using `<h2>` here while you're using `<h3>` above for the same kind of headline? Note that this causes those two to have different styles applied to them. https://codereview.adblockplus.org/29333819/diff/29334296/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode95 options.js:95: table.childNodes[this.items.indexOf(item)]); Detail: Please use braces around it if you occupy more than a single line. Note that we usually indent a continuing line with two additional double-spaces. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode128 options.js:128: var staticElements = []; The above two variables are no longer used. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode323 options.js:323: collections.allLangs.hasId(tableId))) I've been thinking about this again. Wouldn't you agree that all of that should be handled by the Collection? https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode384 options.js:384: for (var param in subscription) Detail: It's a property, not a parameter. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode385 options.js:385: if (param != "title") Why did you specifically exclude the "title" property. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode473 options.js:473: element = element.parentNode; This may cause an exception if there is no parent element that has that attribute. In the upcoming review for #2409 I was planning to introduce the following function which you could use here as well so feel free to implement it here already or improve this function based on it. function findData(element, dataName, returnElement) { while (element) { if (element.hasAttribute("data-" + dataName)) return returnElement ? element : element.getAttribute("data-" + dataName); element = element.parentElement; } return null; } https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode567 options.js:567: getParentAccessElement(element).classList.remove("context"); Why not just add a "close-context-menu" action to the list elements directly instead of introducing "website" and "source" actions and manually add it to other ones? https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode575 options.js:575: case "delete": Detail: This message name doesn't describe what it does (i.e. what's deleted). Therefore I'd suggest renaming it to "remove-subscription". https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode901 options.js:901: collections.blockingLists.removeItem(knownSubscription); Detail: Semantically, updating the UI should happen after the storage was modified so I'd suggest moving this code one line up. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode990 options.js:990: filter: ["added", "disabled", "homepage", "lastDownload", "removed", "title", "updated"] Detail: You removed the handling for "updated" so I guess you can remove it from here. https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css#ne... skin/options.css:347: .table.list li:nth-child(odd), I guess you'd also want to do change it here. https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css#ne... skin/options.css:972: div.container::before Was it intentional to call the class "container" even though it's only used to contain the context menu? I'm asking because before it was selected via `div[role="context-menu"]` which was more descriptive than it is here.
New patch uploaded https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29333819/diff/29333820/messageResponder.js... messageResponder.js:50: "downloadStatus", "lastDownload", "homepage", "lastSuccess", "title", "url"]); On 2016/01/25 15:40:27, Thomas Greiner wrote: > On 2016/01/22 09:55:06, saroyanm wrote: > > On 2016/01/19 11:27:27, Thomas Greiner wrote: > > > Do we want "lastDownload", "lastSuccess" or both? > > > > In current option page, seems like we are using lastDownload, so I'll keep > > lastDownload for now, in case we will need lastSuccess in future we can always > > add. > > There's already a ticket requesting for this information to be shown > additionally in case of an error (see > https://issues.adblockplus.org/ticket/2848) so we can work on it separately. Acknowledged. https://codereview.adblockplus.org/29333819/diff/29333820/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcode46 options.html:46: <li id="tab-advanced" data-action="switch-tab" data-tab="advanced,filter-lists"> On 2016/01/25 15:40:27, Thomas Greiner wrote: > On 2016/01/22 09:55:07, saroyanm wrote: > > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > > Detail: "abc,def" looks like a list of elements but it seems that you want > to > > > indicate that on page "abc" it should navigate to "def". Therefore I'd > suggest > > > to use either "abc/def" or "abc.def" to indicate that hierarchy. > > No, I would like to indicate that the list of tabs should be active after > > triggering "switch-tab" action, same way we indicate list of actions, when we > > would like to trigger several actions. > > But if you think that we need to have hierarchical model of tab management, we > > can have it, I just can't see benefit. What you think ? > > I see what you mean but in this case they are dependent on each other since > switching to tab "filter-lists" doesn't do anything unless you also switch to > tab "advanced" so in this case "filter-lists" is dependent on "advanced". > > Note that this is also relevant for deep-linking which I'll introduce in #2409 > that allows us to link to a section of the options page that may be in a > different tab. Therefore my suggestion would be to use > `data-tab="advanced-filterLists"` so that we can link to it via > `options.html#advanced-filterLists` and apply styles via > `body[data-tab|="advanced"]` and `body[data-tab|="filterLists"]`. Done. https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:263: <input type="checkbox" id="own-list" checked="true" disabled="true" /> On 2016/01/25 15:40:27, Thomas Greiner wrote: > On 2016/01/22 09:55:08, saroyanm wrote: > > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > > This checkbox doesn't do anything so either implement the en-/disabling of > > this > > > custom subscription or visually indicate that this checkbox is disabled. > > > > Not sure if it looks good, if you think it's ugly or doesn't fit to current > > review I'll implement the toggle. > > Also make sense to decide whether we need to have context menu for this item > or > > not, anyway I'll suggest to revisit user's filter list item management > > separately. > > I don't mind to do it in a separate review but we should definitely work on > that. Acknowledged. https://codereview.adblockplus.org/29333819/diff/29333820/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode124 options.js:124: var staticElements = []; On 2016/01/25 15:40:27, Thomas Greiner wrote: > On 2016/01/22 09:55:10, saroyanm wrote: > > On 2016/01/19 11:27:29, Thomas Greiner wrote: > > > This logic is quite complicated. Assuming that you merely want to know at > > which > > > position elements should be inserted you could go with the following > approach: > > > > > > 1. Create a document fragment: document.createDocumentFragment() > > > 2. Append list elements to the document fragment: fragment.appendChild() > > > 3. Insert the document fragment at the appropriate position: > > > template.parentNode.insertBefore(fragment, template) > > > > > > Obviously, that means that you won't be able to clear it with > `table.innerHTML > > = > > > ""` anymore and that you therefore have to remove each element individually > > but > > > that should be fine since we're only calling this method in > `populateLists()`. > > > > Currently we are not using createDocumentFragment to add List item, I've > updated > > current removal implementation to only remove the dynamic list items, I also > > need to remove text nodes otherwise it's becoming messy to always determine > the > > type of the node. > > You could use `Element.nextElementSibling` to avoid other kinds of nodes. So > checking for the "static" class and `<template>` elements should be sufficient. > > Alternatively, you could use `table.children` to get a list of all child > elements. > > In any case you need to be careful since you're removing the element from the > DOM. The safest approach would be to use > `Array.prototype.slice.call(table.children)` and loop over the items in the > array. I assume while you didn't commented under updated code, that mean you are okey with the implementation in patch set "#2" https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode175 options.js:175: var timedate = i18n_timeDateStrings(subscription.lastDownload * 1000); On 2016/01/25 15:40:28, Thomas Greiner wrote: > On 2016/01/22 09:55:10, saroyanm wrote: > > On 2016/01/19 11:27:29, Thomas Greiner wrote: > > > This function returns the localized date-time string which is not what > you're > > > looking for because it will differ (e.g. "1970/30/1" or "1970/1/30" or > > > "1.1.1970") and thereby cause unexpected results and forces you to do all > > those > > > modificiations and checks below. > > > > > > Therefore please don't use this function and instead implement it yourself. > > Feel > > > free to add it to "i18n.js" but note that we cannot remove > > > "i18n_timeDateStrings()" yet because it's still being used in the current > > > options page. > > > > I see, I was trying to make it consistent with old code, didn't noticed the > > localization part thanks, not really sure if we need separate method for this > in > > i18n, because it has nothing to do with localization currently, but I have > > implemented the logic in current function, I think should be enough for now, > but > > let me know if you expect this logic to do something else. > > The mere formatting of the string should be generic enough for inclusion in > "i18n.js", all the other parts should stay here. > > e.g. > > var dateTime = i18n_formatDateTime(date); > dateElement.textContent = dateTime[0]; > timeElement.textContent = dateTime[1]; > > Eventually, we might need something even more generic (e.g. > `i18n_formatDateTime(date, "YYYY-MM-DD hh:mm")`) but that'd probably be too much > effort for this specific use case. > > Anyway, please note that the code you implemented formats the date as > "2016-111-1 1:1" instead of the expected "2016-12-01 01:01". Done. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode215 options.js:215: var subscriptionUrl = e.target.parentNode.getAttribute("data-access"); On 2016/01/25 15:40:28, Thomas Greiner wrote: > On 2016/01/22 09:55:10, saroyanm wrote: > > On 2016/01/19 11:27:29, Thomas Greiner wrote: > > > What about reusing `getParentAccessElement()` since you already created it? > > > > Done. > > I don't see that you changed anything here. Now for sure, thanks. https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29333820/skin/options.css#ne... skin/options.css:668: #filter-lists .table li On 2016/01/25 15:40:28, Thomas Greiner wrote: > On 2016/01/22 09:55:12, saroyanm wrote: > > On 2016/01/19 11:27:30, Thomas Greiner wrote: > > > I noticed that long filter list titles cause the line's height to expand. > > Please > > > check back with Sven whether that is intended or whether (a) the title > should > > be > > > cut off or (b) the table's width should increase. > > > > Currently it's consistent with other list elements, I'll suggest keep it like > > this and update that with other list items, because feels like they should > > behave somehow similar. > > Note that the behavior changes with https://codereview.adblockplus.org/29333262/ > but fine with me for now since this change hasn't landed. Acknowledged. https://codereview.adblockplus.org/29333819/diff/29334296/background.js File background.js (right): https://codereview.adblockplus.org/29333819/diff/29334296/background.js#newco... background.js:75: this.lastDownload = 1234; On 2016/01/25 15:40:29, Thomas Greiner wrote: > Since you've added the setter for "lastDownload" this will trigger the > "subscription.lastDownload" event to be triggered whenever a subscription is > added which, however, should only happen when a subscription is downloaded. > > Changing it to `this._lastDownload = 1234;` should be sufficient. Done. https://codereview.adblockplus.org/29333819/diff/29334296/i18n.js File i18n.js (right): https://codereview.adblockplus.org/29333819/diff/29334296/i18n.js#newcode68 i18n.js:68: function addI18nStringsToElements(nodes) On 2016/01/25 15:40:29, Thomas Greiner wrote: > Detail: You're calling them "elements" everywhere else so why go with "node" > here? Done. https://codereview.adblockplus.org/29333819/diff/29334296/i18n.js#newcode85 i18n.js:85: addI18nStringsToElements(document.querySelectorAll("[class^='i18n_']")); On 2016/01/25 15:40:29, Thomas Greiner wrote: > Detail: You're always calling this function using the same selector so to avoid > duplication I'd suggest instead only passing the container element and querying > for elements inside `addI18nStringsToElements()`. Done. https://codereview.adblockplus.org/29333819/diff/29334296/i18n.js#newcode86 i18n.js:86: var templates = document.querySelectorAll("template"); On 2016/01/25 15:40:30, Thomas Greiner wrote: > Detail: Please add a short comment to explain why this is necessary. Done. https://codereview.adblockplus.org/29333819/diff/29334296/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29333819/diff/29334296/locale/en-US/option... locale/en-US/options.json:158: "options_tableCol_name": { On 2016/01/25 15:40:30, Thomas Greiner wrote: > Detail: I just noticed that "column" is abbreviated which may be a bit confusing > so what about renaming it to "options_column_name"? Done. https://codereview.adblockplus.org/29333819/diff/29334296/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29334296/options.html#newcod... options.html:237: <div id="filter-lists"> On 2016/01/25 15:40:31, Thomas Greiner wrote: > Detail: It's unclear what the difference between the IDs "blocking-lists", > "filter-lists" and "blocking-lists-table" here so I'd suggest renaming them. > > e.g. something along the lines of "filterLists" and "filterLists-container" and > "filterLists-list" (also since we no longer call them "blocking lists" in the > texts) Done, I also updated texts that contain blocking list, translation string names and changed collection name to not mention in the name blockingList so we can be consistent with naming and have less confusion about naming. https://codereview.adblockplus.org/29333819/diff/29334296/options.html#newcod... options.html:248: <a href="#" data-action="open-context-menu" class="arrow"></a> On 2016/01/25 15:40:30, Thomas Greiner wrote: > Detail: No need to set the "href" attribute here since we're not linking > anywhere. Done, also removed from other non linking anchors on Advanced page. https://codereview.adblockplus.org/29333819/diff/29334296/options.html#newcod... options.html:263: <input type="checkbox" id="own-list" checked="true" disabled="true" /> On 2016/01/25 15:40:31, Thomas Greiner wrote: > Detail: It doesn't seem you're using this ID here so please remove it. Done. https://codereview.adblockplus.org/29333819/diff/29334296/options.html#newcod... options.html:282: <h2 id="custom-filters-header" class="i18n_options_customFilters_title"></h2> On 2016/01/25 15:40:30, Thomas Greiner wrote: > Why are you using `<h2>` here while you're using `<h3>` above for the same kind > of headline? Note that this causes those two to have different styles applied to > them. Done. https://codereview.adblockplus.org/29333819/diff/29334296/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode95 options.js:95: table.childNodes[this.items.indexOf(item)]); On 2016/01/25 15:40:32, Thomas Greiner wrote: > Detail: Please use braces around it if you occupy more than a single line. > > Note that we usually indent a continuing line with two additional double-spaces. Done. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode128 options.js:128: var staticElements = []; On 2016/01/25 15:40:33, Thomas Greiner wrote: > The above two variables are no longer used. Done. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode323 options.js:323: collections.allLangs.hasId(tableId))) On 2016/01/25 15:40:33, Thomas Greiner wrote: > I've been thinking about this again. Wouldn't you agree that all of that should > be handled by the Collection? I assume you are referring to the "Collection.prototype.updateItem" from https://codereview.adblockplus.org/29332808/ While the referencing review is almost done, I think make sense to rebase current review after pushing the changes, I'll check to finish with that review quick. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode384 options.js:384: for (var param in subscription) On 2016/01/25 15:40:31, Thomas Greiner wrote: > Detail: It's a property, not a parameter. Done. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode385 options.js:385: if (param != "title") On 2016/01/25 15:40:31, Thomas Greiner wrote: > Why did you specifically exclude the "title" property. To use titles specified in Recommendation, so the subscriptions in subscriptionsMap will always have the initial title, anyway we need to update the title as soon "title" action is triggered, otherwise we can't update the title when lastDownload is triggered as in current situation. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode473 options.js:473: element = element.parentNode; On 2016/01/25 15:40:33, Thomas Greiner wrote: > This may cause an exception if there is no parent element that has that > attribute. > > In the upcoming review for #2409 I was planning to introduce the following > function which you could use here as well so feel free to implement it here > already or improve this function based on it. > > function findData(element, dataName, returnElement) > { > while (element) > { > if (element.hasAttribute("data-" + dataName)) > return returnElement ? element : element.getAttribute("data-" + dataName); > > element = element.parentElement; > } > return null; > } Like it, thanks. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode567 options.js:567: getParentAccessElement(element).classList.remove("context"); On 2016/01/25 15:40:33, Thomas Greiner wrote: > Why not just add a "close-context-menu" action to the list elements directly > instead of introducing "website" and "source" actions and manually add it to > other ones? Done. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode575 options.js:575: case "delete": On 2016/01/25 15:40:33, Thomas Greiner wrote: > Detail: This message name doesn't describe what it does (i.e. what's deleted). > Therefore I'd suggest renaming it to "remove-subscription". Done. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode901 options.js:901: collections.blockingLists.removeItem(knownSubscription); On 2016/01/25 15:40:38, Thomas Greiner wrote: > Detail: Semantically, updating the UI should happen after the storage was > modified so I'd suggest moving this code one line up. Done. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode990 options.js:990: filter: ["added", "disabled", "homepage", "lastDownload", "removed", "title", "updated"] On 2016/01/25 15:40:32, Thomas Greiner wrote: > Detail: You removed the handling for "updated" so I guess you can remove it from > here. Done. https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css#ne... skin/options.css:347: .table.list li:nth-child(odd), On 2016/01/25 15:40:38, Thomas Greiner wrote: > I guess you'd also want to do change it here. Done. https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css#ne... skin/options.css:972: div.container::before On 2016/01/25 15:40:38, Thomas Greiner wrote: > Was it intentional to call the class "container" even though it's only used to > contain the context menu? I'm asking because before it was selected via > `div[role="context-menu"]` which was more descriptive than it is here. Not sure why I named it so, but I agree with you that it's not descriptive at all, update.
https://codereview.adblockplus.org/29333819/diff/29333820/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:248: <a href="#" data-action="open-context-menu" class="arrow"></a> On 2016/01/22 09:55:07, saroyanm wrote: > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > I noticed a bunch of positioning styles in "options.css". Presumably, that's > > because the DIV below is not directly linked to this anchor tag. Therefore I'd > > suggest moving the DIV inside the anchor tag to simplify the styling code. > > > > Maybe you could even get rid of the DIV around those two elements. > > The problem with double div is that I don't have a fixed width, so I'm > possitioning -50% the inner div inside absolute div, so I can have flexible > layout. > Also having the anchor outside I can change the visibility of sibling div. I tried it myself and didn't notice any issues. What you may have encountered is that the browser was moving your elements outside of the `<a>` element because those are not allowed to have block-elements inside them. The following structure worked for me: <div class="context-menu"> <div class="arrow"> <div class="context-menu-container"> <div class="content"> <a class="update-now"></a> <a class="website"></a> <a class="source"></a> <a class="delete"></a> </div> </div> </div> </div> https://codereview.adblockplus.org/29333819/diff/29333820/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode124 options.js:124: var staticElements = []; On 2016/01/26 18:36:15, saroyanm wrote: > On 2016/01/25 15:40:27, Thomas Greiner wrote: > > On 2016/01/22 09:55:10, saroyanm wrote: > > > On 2016/01/19 11:27:29, Thomas Greiner wrote: > > > > This logic is quite complicated. Assuming that you merely want to know at > > > which > > > > position elements should be inserted you could go with the following > > approach: > > > > > > > > 1. Create a document fragment: document.createDocumentFragment() > > > > 2. Append list elements to the document fragment: fragment.appendChild() > > > > 3. Insert the document fragment at the appropriate position: > > > > template.parentNode.insertBefore(fragment, template) > > > > > > > > Obviously, that means that you won't be able to clear it with > > `table.innerHTML > > > = > > > > ""` anymore and that you therefore have to remove each element > individually > > > but > > > > that should be fine since we're only calling this method in > > `populateLists()`. > > > > > > Currently we are not using createDocumentFragment to add List item, I've > > updated > > > current removal implementation to only remove the dynamic list items, I also > > > need to remove text nodes otherwise it's becoming messy to always determine > > the > > > type of the node. > > > > You could use `Element.nextElementSibling` to avoid other kinds of nodes. So > > checking for the "static" class and `<template>` elements should be > sufficient. > > > > Alternatively, you could use `table.children` to get a list of all child > > elements. > > > > In any case you need to be careful since you're removing the element from the > > DOM. The safest approach would be to use > > `Array.prototype.slice.call(table.children)` and loop over the items in the > > array. > > I assume while you didn't commented under updated code, that mean you are okey > with the implementation in patch set "#2" Thanks for reminding me. I wrote down some parts that I still wanted to comment on but forgot about those so I added them now. https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode557 options.js:557: case "open-context-menu": On 2016/01/22 09:55:09, saroyanm wrote: > On 2016/01/19 11:27:29, Thomas Greiner wrote: > > We should improve this mechanism because it requires quite a bit of extra work > > to get the UX right. > > > > My suggestion would be to listen to clicks: > > - click on subscription title: > > - if context menu hidden: show context menu > > - else: hide context menu > > - click outside of context menu: hide context menu > > - click on action item in context menu: hide context menu > > - click somewhere else in context menu: ignore > > > > That would also be consistent with other context menus (e.g. right-click > context > > menu). > > Done, but bit hacky, not sure if it's what you mean. Better but what about replacing it with "toggle-context-menu"? Because right now it doesn't close again when clicking on the link or arrow again. https://codereview.adblockplus.org/29333819/diff/29334296/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29334296/options.html#newcod... options.html:237: <div id="filter-lists"> On 2016/01/26 18:36:17, saroyanm wrote: > On 2016/01/25 15:40:31, Thomas Greiner wrote: > > Detail: It's unclear what the difference between the IDs "blocking-lists", > > "filter-lists" and "blocking-lists-table" here so I'd suggest renaming them. > > > > e.g. something along the lines of "filterLists" and "filterLists-container" > and > > "filterLists-list" (also since we no longer call them "blocking lists" in the > > texts) > > Done, I also updated texts that contain blocking list, translation string names > and changed collection name to not mention in the name blockingList so we can be > consistent with naming and have less confusion about naming. Awesome, thanks! https://codereview.adblockplus.org/29333819/diff/29334296/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode84 options.js:84: updateBlockingList(listItem, item); This doesn't belong into Collection since it's specific to one table. Therefore I'd suggest moving it into the observer function. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode95 options.js:95: table.childNodes[this.items.indexOf(item)]); On 2016/01/26 18:36:19, saroyanm wrote: > On 2016/01/25 15:40:32, Thomas Greiner wrote: > > Detail: Please use braces around it if you occupy more than a single line. > > > > Note that we usually indent a continuing line with two additional > double-spaces. > > Done. Detail: What I meant with "indent using double-spaces" was to use four spaces instead of the usual two when starting a new line: abcdef(ghi ....jklmno) instead of abcdef(ghi ..jklmno) Because that way we can write longer lines while still making clear that the second line is part of the previous statement and not another independent statement. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode323 options.js:323: collections.allLangs.hasId(tableId))) On 2016/01/26 18:36:18, saroyanm wrote: > On 2016/01/25 15:40:33, Thomas Greiner wrote: > > I've been thinking about this again. Wouldn't you agree that all of that > should > > be handled by the Collection? > > I assume you are referring to the "Collection.prototype.updateItem" from > https://codereview.adblockplus.org/29332808/ > While the referencing review is almost done, I think make sense to rebase > current review after pushing the changes, I'll check to finish with that review > quick. I guess you're right. We can leave it here for now and clean it up afterwards by moving the relevant parts into Collection. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode385 options.js:385: if (param != "title") On 2016/01/26 18:36:19, saroyanm wrote: > On 2016/01/25 15:40:31, Thomas Greiner wrote: > > Why did you specifically exclude the "title" property. > > To use titles specified in Recommendation, so the subscriptions in > subscriptionsMap will always have the initial title, anyway we need to update > the title as soon "title" action is triggered, otherwise we can't update the > title when lastDownload is triggered as in current situation. Right, forgot about that case. It's actually trickier than that because "subscriptions.title" can be triggered at any point (e.g. subscription updates in the background and includes a different title than before). So as soon as we implement the updating of titles we should somehow make sure that the titles from recommended subscriptions stay unchanged. Out of the scope of this review, however, so let's tackle this separately. https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css#ne... skin/options.css:904: .context-menu According to the style guide there should be 6px between the title and the arrow. Also note that the cursor on the arrow (default) is different than the one on the title (pointer). https://codereview.adblockplus.org/29333819/diff/29334623/i18n.js File i18n.js (right): https://codereview.adblockplus.org/29333819/diff/29334623/i18n.js#newcode87 i18n.js:87: // Content of Template is not rendered on tuntime so we need to add Typo: Replace "tuntime" with "runtime". https://codereview.adblockplus.org/29333819/diff/29334623/i18n.js#newcode111 i18n.js:111: var dateParts = [date.getFullYear(), date.getMonth()+1, date.getDate(), Detail: Missing whitespace around `+` operator. https://codereview.adblockplus.org/29333819/diff/29334623/i18n.js#newcode116 i18n.js:116: dateParts[i] = "0" + dateParts[i]; Detail: This is basically what `Array.prototype.map` was meant for so what about using it instead to make this part a bit cleaner? https://codereview.adblockplus.org/29333819/diff/29334623/i18n.js#newcode118 i18n.js:118: return [dateParts.splice(0, 3).join("-"), dateParts.splice(0, 2).join(":")]; The first `splice()` removes elements from `dateParts` so the second `splice()` is not necessary. Therefore you could either remove the second `splice()` or replace `splice()` with `slice()`. https://codereview.adblockplus.org/29333819/diff/29334623/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29333819/diff/29334623/locale/en-US/option... locale/en-US/options.json:148: "message": "Filter list" Detail: This text should be plural according to the designs. https://codereview.adblockplus.org/29333819/diff/29334623/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29334623/options.html#newcod... options.html:250: <div class="content" data-action="no-action"> Why is there an action called "no-action"? https://codereview.adblockplus.org/29333819/diff/29334623/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29334623/options.js#newcode133 options.js:133: element.nodeType == 3) Again, checking for the "static" class when using `Element.nextElementSibling` should be sufficient or not? https://codereview.adblockplus.org/29333819/diff/29334623/options.js#newcode486 options.js:486: var context = document.querySelector(".context"); Detail: This class name is not very descriptive of its effects so it'd be great if you could call it something like "show-context" or "show-context-menu" instead. https://codereview.adblockplus.org/29333819/diff/29334623/options.js#newcode489 options.js:489: return; The context menu is also supposed to go away whenever you click on an element that has an action attached to it so it probably makes sense to move this outside of the switch-statement to the bottom of the click handler. e.g. // close context menu if (action == "open-context-menu") { // open context menu unless it is the same one that we just closed above } https://codereview.adblockplus.org/29333819/diff/29334623/options.js#newcode559 options.js:559: var contextMenu = listItem.querySelector(".content"); Detail: This variable is not being used. https://codereview.adblockplus.org/29333819/diff/29334623/options.js#newcode566 options.js:566: url: findParentData(element, "access") Detail: While the last parameter is optional in such cases, you did explicitly set it in all other cases. I don't mind either way but it should be consistent.
Rebased, will address the comments soon as well.
Just a minor comment about the rebasing. https://codereview.adblockplus.org/29333819/diff/29334799/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29334799/options.js#newcode83 options.js:83: updateBlockingList(listItem, item); Detail: Don't use `\t` for whitespaces.
New patch uploaded, Note: I've generated the sprite again from xcf again, so you can see some changes in rietveld on skin/options-sprite.png, but It should be same. https://codereview.adblockplus.org/29333819/diff/29333820/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:248: <a href="#" data-action="open-context-menu" class="arrow"></a> On 2016/01/27 17:16:56, Thomas Greiner wrote: > On 2016/01/22 09:55:07, saroyanm wrote: > > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > > I noticed a bunch of positioning styles in "options.css". Presumably, that's > > > because the DIV below is not directly linked to this anchor tag. Therefore > I'd > > > suggest moving the DIV inside the anchor tag to simplify the styling code. > > > > > > Maybe you could even get rid of the DIV around those two elements. > > > > The problem with double div is that I don't have a fixed width, so I'm > > possitioning -50% the inner div inside absolute div, so I can have flexible > > layout. > > Also having the anchor outside I can change the visibility of sibling div. > > I tried it myself and didn't notice any issues. What you may have encountered is > that the browser was moving your elements outside of the `<a>` element because > those are not allowed to have block-elements inside them. > > The following structure worked for me: > > <div class="context-menu"> > <div class="arrow"> > <div class="context-menu-container"> > <div class="content"> > <a class="update-now"></a> > <a class="website"></a> > <a class="source"></a> > <a class="delete"></a> > </div> > </div> > </div> > </div> Shouldn't the context menu be relative to anchor element ? I think I changed the implementation from some point, can you please comment regarding this in new patch in case you still see some issues. But maybe currently it's implemented in same way I think I forgot to comment about that in some previous reviews. https://codereview.adblockplus.org/29333819/diff/29333820/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.js#newcode557 options.js:557: case "open-context-menu": On 2016/01/27 17:16:56, Thomas Greiner wrote: > On 2016/01/22 09:55:09, saroyanm wrote: > > On 2016/01/19 11:27:29, Thomas Greiner wrote: > > > We should improve this mechanism because it requires quite a bit of extra > work > > > to get the UX right. > > > > > > My suggestion would be to listen to clicks: > > > - click on subscription title: > > > - if context menu hidden: show context menu > > > - else: hide context menu > > > - click outside of context menu: hide context menu > > > - click on action item in context menu: hide context menu > > > - click somewhere else in context menu: ignore > > > > > > That would also be consistent with other context menus (e.g. right-click > > context > > > menu). > > > > Done, but bit hacky, not sure if it's what you mean. > > Better but what about replacing it with "toggle-context-menu"? Because right now > it doesn't close again when clicking on the link or arrow again. Done. https://codereview.adblockplus.org/29333819/diff/29334296/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode84 options.js:84: updateBlockingList(listItem, item); On 2016/01/27 17:16:57, Thomas Greiner wrote: > This doesn't belong into Collection since it's specific to one table. Therefore > I'd suggest moving it into the observer function. We are calling this method on observer as well, This method is used to update the date/time and links of the filterList item, but in general it should be part of updateItem method, I've added that implementation to updateItem method, hope it's more clear now. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode95 options.js:95: table.childNodes[this.items.indexOf(item)]); On 2016/01/27 17:16:58, Thomas Greiner wrote: > On 2016/01/26 18:36:19, saroyanm wrote: > > On 2016/01/25 15:40:32, Thomas Greiner wrote: > > > Detail: Please use braces around it if you occupy more than a single line. > > > > > > Note that we usually indent a continuing line with two additional > > double-spaces. > > > > Done. > > Detail: What I meant with "indent using double-spaces" was to use four spaces > instead of the usual two when starting a new line: > > abcdef(ghi > ....jklmno) > > instead of > > abcdef(ghi > ..jklmno) > > Because that way we can write longer lines while still making clear that the > second line is part of the previous statement and not another independent > statement. Got it, sorry for that, I was thinking that we need to align the second line with the related expression on line above for readability. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode323 options.js:323: collections.allLangs.hasId(tableId))) On 2016/01/27 17:16:57, Thomas Greiner wrote: > On 2016/01/26 18:36:18, saroyanm wrote: > > On 2016/01/25 15:40:33, Thomas Greiner wrote: > > > I've been thinking about this again. Wouldn't you agree that all of that > > should > > > be handled by the Collection? > > > > I assume you are referring to the "Collection.prototype.updateItem" from > > https://codereview.adblockplus.org/29332808/ > > While the referencing review is almost done, I think make sense to rebase > > current review after pushing the changes, I'll check to finish with that > review > > quick. > > I guess you're right. We can leave it here for now and clean it up afterwards by > moving the relevant parts into Collection. Done, hope now it's more clear. https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode385 options.js:385: if (param != "title") On 2016/01/27 17:16:57, Thomas Greiner wrote: > On 2016/01/26 18:36:19, saroyanm wrote: > > On 2016/01/25 15:40:31, Thomas Greiner wrote: > > > Why did you specifically exclude the "title" property. > > > > To use titles specified in Recommendation, so the subscriptions in > > subscriptionsMap will always have the initial title, anyway we need to update > > the title as soon "title" action is triggered, otherwise we can't update the > > title when lastDownload is triggered as in current situation. > > Right, forgot about that case. > > It's actually trickier than that because "subscriptions.title" can be triggered > at any point (e.g. subscription updates in the background and includes a > different title than before). > So as soon as we implement the updating of titles we should somehow make sure > that the titles from recommended subscriptions stay unchanged. > > Out of the scope of this review, however, so let's tackle this separately. Acknowledged. https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css#ne... skin/options.css:904: .context-menu On 2016/01/27 17:16:58, Thomas Greiner wrote: > According to the style guide there should be 6px between the title and the > arrow. > > Also note that the cursor on the arrow (default) is different than the one on > the title (pointer). Sorry, from some point I missed the style-guide and were using mockup, Done. https://codereview.adblockplus.org/29333819/diff/29334623/i18n.js File i18n.js (right): https://codereview.adblockplus.org/29333819/diff/29334623/i18n.js#newcode87 i18n.js:87: // Content of Template is not rendered on tuntime so we need to add On 2016/01/27 17:16:59, Thomas Greiner wrote: > Typo: Replace "tuntime" with "runtime". Done. https://codereview.adblockplus.org/29333819/diff/29334623/i18n.js#newcode111 i18n.js:111: var dateParts = [date.getFullYear(), date.getMonth()+1, date.getDate(), On 2016/01/27 17:16:58, Thomas Greiner wrote: > Detail: Missing whitespace around `+` operator. Done. https://codereview.adblockplus.org/29333819/diff/29334623/i18n.js#newcode116 i18n.js:116: dateParts[i] = "0" + dateParts[i]; On 2016/01/27 17:16:59, Thomas Greiner wrote: > Detail: This is basically what `Array.prototype.map` was meant for so what about > using it instead to make this part a bit cleaner? God bless development without IE8! Done. https://codereview.adblockplus.org/29333819/diff/29334623/i18n.js#newcode118 i18n.js:118: return [dateParts.splice(0, 3).join("-"), dateParts.splice(0, 2).join(":")]; On 2016/01/27 17:16:58, Thomas Greiner wrote: > The first `splice()` removes elements from `dateParts` so the second `splice()` > is not necessary. > > Therefore you could either remove the second `splice()` or replace `splice()` > with `slice()`. Done. https://codereview.adblockplus.org/29333819/diff/29334623/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29333819/diff/29334623/locale/en-US/option... locale/en-US/options.json:148: "message": "Filter list" On 2016/01/27 17:16:59, Thomas Greiner wrote: > Detail: This text should be plural according to the designs. Done. https://codereview.adblockplus.org/29333819/diff/29334623/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29334623/options.html#newcod... options.html:250: <div class="content" data-action="no-action"> On 2016/01/27 17:16:59, Thomas Greiner wrote: > Why is there an action called "no-action"? Done, left over from old implementation, thanks for noticing. https://codereview.adblockplus.org/29333819/diff/29334623/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29334623/options.js#newcode133 options.js:133: element.nodeType == 3) On 2016/01/27 17:17:00, Thomas Greiner wrote: > Again, checking for the "static" class when using `Element.nextElementSibling` > should be sufficient or not? Done. https://codereview.adblockplus.org/29333819/diff/29334623/options.js#newcode486 options.js:486: var context = document.querySelector(".context"); On 2016/01/27 17:17:00, Thomas Greiner wrote: > Detail: This class name is not very descriptive of its effects so it'd be great > if you could call it something like "show-context" or "show-context-menu" > instead. Done. https://codereview.adblockplus.org/29333819/diff/29334623/options.js#newcode489 options.js:489: return; On 2016/01/27 17:17:00, Thomas Greiner wrote: > The context menu is also supposed to go away whenever you click on an element > that has an action attached to it so it probably makes sense to move this > outside of the switch-statement to the bottom of the click handler. > > e.g. > > // close context menu > > if (action == "open-context-menu") > { > // open context menu unless it is the same one that we just closed above > } Not really sure that I understood what you mean, but changed the implementation, please let me know if it's still needs to be changed. https://codereview.adblockplus.org/29333819/diff/29334623/options.js#newcode559 options.js:559: var contextMenu = listItem.querySelector(".content"); On 2016/01/27 17:17:00, Thomas Greiner wrote: > Detail: This variable is not being used. Done. https://codereview.adblockplus.org/29333819/diff/29334623/options.js#newcode566 options.js:566: url: findParentData(element, "access") On 2016/01/27 17:17:00, Thomas Greiner wrote: > Detail: While the last parameter is optional in such cases, you did explicitly > set it in all other cases. I don't mind either way but it should be consistent. Done.
https://codereview.adblockplus.org/29333819/diff/29333820/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:248: <a href="#" data-action="open-context-menu" class="arrow"></a> On 2016/01/28 17:00:10, saroyanm wrote: > On 2016/01/27 17:16:56, Thomas Greiner wrote: > > On 2016/01/22 09:55:07, saroyanm wrote: > > > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > > > I noticed a bunch of positioning styles in "options.css". Presumably, > that's > > > > because the DIV below is not directly linked to this anchor tag. Therefore > > I'd > > > > suggest moving the DIV inside the anchor tag to simplify the styling code. > > > > > > > > Maybe you could even get rid of the DIV around those two elements. > > > > > > The problem with double div is that I don't have a fixed width, so I'm > > > possitioning -50% the inner div inside absolute div, so I can have flexible > > > layout. > > > Also having the anchor outside I can change the visibility of sibling div. > > > > I tried it myself and didn't notice any issues. What you may have encountered > is > > that the browser was moving your elements outside of the `<a>` element because > > those are not allowed to have block-elements inside them. > > > > The following structure worked for me: > > > > <div class="context-menu"> > > <div class="arrow"> > > <div class="context-menu-container"> > > <div class="content"> > > <a class="update-now"></a> > > <a class="website"></a> > > <a class="source"></a> > > <a class="delete"></a> > > </div> > > </div> > > </div> > > </div> > > Shouldn't the context menu be relative to anchor element ? According to https://issues.adblockplus.org/attachment/ticket/1526/advanced_3_77.png it should be relative to the arrow. > But maybe currently it's implemented in same way I think I forgot to comment > about that in some previous reviews. I didn't see any code change that would have addressed this. https://codereview.adblockplus.org/29333819/diff/29334296/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29334296/options.js#newcode84 options.js:84: updateBlockingList(listItem, item); On 2016/01/28 17:00:11, saroyanm wrote: > On 2016/01/27 17:16:57, Thomas Greiner wrote: > > This doesn't belong into Collection since it's specific to one table. > Therefore > > I'd suggest moving it into the observer function. > > We are calling this method on observer as well, This method is used to update > the date/time and links of the filterList item, but in general it should be part > of updateItem method, I've added that implementation to updateItem method, hope > it's more clear now. This code is only relevant for subscriptions. However, Collection also manages filters which don't specify source, homepage, etc. Therefore it should be separate from Collection. However, I'm fine with how it looks right now to centralize logic. https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css#ne... skin/options.css:904: .context-menu On 2016/01/28 17:00:12, saroyanm wrote: > On 2016/01/27 17:16:58, Thomas Greiner wrote: > > According to the style guide there should be 6px between the title and the > > arrow. > > > > Also note that the cursor on the arrow (default) is different than the one on > > the title (pointer). > > Sorry, from some point I missed the style-guide and were using mockup, Done. There's still no 6px between the title and the arrow or am I missing something? https://codereview.adblockplus.org/29333819/diff/29334623/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29334623/options.js#newcode489 options.js:489: return; On 2016/01/28 17:00:14, saroyanm wrote: > On 2016/01/27 17:17:00, Thomas Greiner wrote: > > The context menu is also supposed to go away whenever you click on an element > > that has an action attached to it so it probably makes sense to move this > > outside of the switch-statement to the bottom of the click handler. > > > > e.g. > > > > // close context menu > > > > if (action == "open-context-menu") > > { > > // open context menu unless it is the same one that we just closed above > > } > > Not really sure that I understood what you mean, but changed the implementation, > please let me know if it's still needs to be changed. Here's a more detailed and fleshed-out version: function onClick(e) { ... var context = document.querySelector(".show-context-menu"); if (context) context.classList.remove("show-context-menu"); for (...) { switch (actions[i]) { ... case "open-context-menu": var listItem = findParentData(element, "access", true); if (listItem != context) listItem.classList.add("show-context-menu"); break; ... } } ... } That way you should be able to ensure that the context menu always goes away and only comes up when clicking on a different subscription title.
New patch uploaded. https://codereview.adblockplus.org/29333819/diff/29333820/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29333820/options.html#newcod... options.html:248: <a href="#" data-action="open-context-menu" class="arrow"></a> On 2016/01/29 17:48:07, Thomas Greiner wrote: > On 2016/01/28 17:00:10, saroyanm wrote: > > On 2016/01/27 17:16:56, Thomas Greiner wrote: > > > On 2016/01/22 09:55:07, saroyanm wrote: > > > > On 2016/01/19 11:27:28, Thomas Greiner wrote: > > > > > I noticed a bunch of positioning styles in "options.css". Presumably, > > that's > > > > > because the DIV below is not directly linked to this anchor tag. > Therefore > > > I'd > > > > > suggest moving the DIV inside the anchor tag to simplify the styling > code. > > > > > > > > > > Maybe you could even get rid of the DIV around those two elements. > > > > > > > > The problem with double div is that I don't have a fixed width, so I'm > > > > possitioning -50% the inner div inside absolute div, so I can have > flexible > > > > layout. > > > > Also having the anchor outside I can change the visibility of sibling div. > > > > > > I tried it myself and didn't notice any issues. What you may have > encountered > > is > > > that the browser was moving your elements outside of the `<a>` element > because > > > those are not allowed to have block-elements inside them. > > > > > > The following structure worked for me: > > > > > > <div class="context-menu"> > > > <div class="arrow"> > > > <div class="context-menu-container"> > > > <div class="content"> > > > <a class="update-now"></a> > > > <a class="website"></a> > > > <a class="source"></a> > > > <a class="delete"></a> > > > </div> > > > </div> > > > </div> > > > </div> > > > > Shouldn't the context menu be relative to anchor element ? > > According to > https://issues.adblockplus.org/attachment/ticket/1526/advanced_3_77.png it > should be relative to the arrow. > > > But maybe currently it's implemented in same way I think I forgot to comment > > about that in some previous reviews. > > I didn't see any code change that would have addressed this. I think I missread the part where you suggested to change anchor tag intially, sorry for that, done. https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29334296/skin/options.css#ne... skin/options.css:904: .context-menu On 2016/01/29 17:48:08, Thomas Greiner wrote: > On 2016/01/28 17:00:12, saroyanm wrote: > > On 2016/01/27 17:16:58, Thomas Greiner wrote: > > > According to the style guide there should be 6px between the title and the > > > arrow. > > > > > > Also note that the cursor on the arrow (default) is different than the one > on > > > the title (pointer). > > > > Sorry, from some point I missed the style-guide and were using mockup, Done. > > There's still no 6px between the title and the arrow or am I missing something? my fault, were thinking about the 6px vertically between two arrows (context-open and context arrow), addec 4px IMO with 6px looks a bit misaligned. https://codereview.adblockplus.org/29333819/diff/29334623/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29334623/options.js#newcode489 options.js:489: return; On 2016/01/29 17:48:09, Thomas Greiner wrote: > On 2016/01/28 17:00:14, saroyanm wrote: > > On 2016/01/27 17:17:00, Thomas Greiner wrote: > > > The context menu is also supposed to go away whenever you click on an > element > > > that has an action attached to it so it probably makes sense to move this > > > outside of the switch-statement to the bottom of the click handler. > > > > > > e.g. > > > > > > // close context menu > > > > > > if (action == "open-context-menu") > > > { > > > // open context menu unless it is the same one that we just closed above > > > } > > > > Not really sure that I understood what you mean, but changed the > implementation, > > please let me know if it's still needs to be changed. > > Here's a more detailed and fleshed-out version: > > function onClick(e) > { > ... > var context = document.querySelector(".show-context-menu"); > if (context) > context.classList.remove("show-context-menu"); > > for (...) > { > switch (actions[i]) > { > ... > case "open-context-menu": > var listItem = findParentData(element, "access", true); > if (listItem != context) > listItem.classList.add("show-context-menu"); > break; > ... > } > } > ... > } > > That way you should be able to ensure that the context menu always goes away and > only comes up when clicking on a different subscription title. Well, that's what I've had implemented as well after your note, beside of the part for checking the context, because I wasn't sure whether the context needs to be hidden after second click on the list item, but yes also added that part.
Only some minor comments. https://codereview.adblockplus.org/29333819/diff/29335133/i18n.js File i18n.js (right): https://codereview.adblockplus.org/29333819/diff/29335133/i18n.js#newcode107 i18n.js:107: // Formats date string to ["yyyy-mm-dd", "mm:ss"] format Detail: Usually, you would write it as "YYYY-MM-DD" to differentiate between months and minutes. https://codereview.adblockplus.org/29333819/diff/29335133/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29335133/options.html#newcod... options.html:273: <span class="icon icon-add"></span><span class="i18n_options_filterList_add"></span> Detail: Please split this up into separate lines for readability. https://codereview.adblockplus.org/29333819/diff/29335133/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29335133/options.js#newcode170 options.js:170: var map = Detail: This content is static so it should be defined outside to avoid declaring it each time when the function gets called. https://codereview.adblockplus.org/29333819/diff/29335133/options.js#newcode187 options.js:187: } What about the else-case here? Currently, this will cause both fields to be empty whenever a new subscription is added until the download finishes. If you look at the current options page it is indicating "Downloading…" which is also shown when updating a subscription. The latter one is completely missing from the new options page so we should fix that as well. https://codereview.adblockplus.org/29333819/diff/29335133/options.js#newcode367 options.js:367: if (!Object.observe) Since `Object.observe()` is not implemented in Safari and older Chrome versions and is going to be removed at some point, have you tested whether the polyfill still works? https://codereview.adblockplus.org/29333819/diff/29335133/options.js#newcode580 options.js:580: case "update-now": Detail: For consistency I'd suggest renaming this to "update-subscription". Also because "update-now" could be confused with "update-all-subscriptions". https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:620: body[data-tab="advanced-allFilterLists"] #content-advanced [data-tab="advanced-allFilterLists"], Detail: Mentioning `#content-advanced` is redundant since the tab name already mentions that it's part of the Advanced tab. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:690: position: relative; Detail: According to the coding style positioning should come before box model-related properties. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:694: margin: 0px 4px; The reason why it looks odd to you to specify `6px` is because there's whitespace between the subscription title and the arrow. Therefore using `4px` instead is merely a workaround for the underlying issue. It seems that specifying `display: flex` on the parent and `vertical-align: middle; margin: auto 6px;` resolves the issue and properly align it vertically. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:695: background-color: transparent; Detail: Why is that necessary? https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:968: border-radius: 3px; Detail: Why did you move the border radius to here? https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:982: div.context-menu .content Detail: Please add `cursor: default` to avoid showing the pointing cursor when hovering over non-clickable elements in the context menu. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:1011: text-decoration: none; Detail: This property is in the wrong spot according to our coding style since it belongs into the "Colors and Typography" category. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:1018: div.context-menu > div a:before Detail: Replace ":before" with "::before" to make it consistent with the rest of the code.
Sorry that took a while. https://codereview.adblockplus.org/29333819/diff/29335133/i18n.js File i18n.js (right): https://codereview.adblockplus.org/29333819/diff/29335133/i18n.js#newcode107 i18n.js:107: // Formats date string to ["yyyy-mm-dd", "mm:ss"] format On 2016/02/01 18:52:35, Thomas Greiner wrote: > Detail: Usually, you would write it as "YYYY-MM-DD" to differentiate between > months and minutes. Done. https://codereview.adblockplus.org/29333819/diff/29335133/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29335133/options.html#newcod... options.html:273: <span class="icon icon-add"></span><span class="i18n_options_filterList_add"></span> On 2016/02/01 18:52:35, Thomas Greiner wrote: > Detail: Please split this up into separate lines for readability. Done. https://codereview.adblockplus.org/29333819/diff/29335133/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29335133/options.js#newcode170 options.js:170: var map = On 2016/02/01 18:52:36, Thomas Greiner wrote: > Detail: This content is static so it should be defined outside to avoid > declaring it each time when the function gets called. Done, also made some other function/variable assignments to improve readability. https://codereview.adblockplus.org/29333819/diff/29335133/options.js#newcode187 options.js:187: } On 2016/02/01 18:52:35, Thomas Greiner wrote: > What about the else-case here? Currently, this will cause both fields to be > empty whenever a new subscription is added until the download finishes. > > If you look at the current options page it is indicating "Downloading…" which is > also shown when updating a subscription. The latter one is completely missing > from the new options page so we should fix that as well. Good point, sorry for that, Done. https://codereview.adblockplus.org/29333819/diff/29335133/options.js#newcode367 options.js:367: if (!Object.observe) On 2016/02/01 18:52:35, Thomas Greiner wrote: > Since `Object.observe()` is not implemented in Safari and older Chrome versions > and is going to be removed at some point, have you tested whether the polyfill > still works? Thanks for pointing that out, apparently we have to check also for the old value of the Object property when using define property, otherwise the set event will be called also in case the value of the property is same. Done. https://codereview.adblockplus.org/29333819/diff/29335133/options.js#newcode580 options.js:580: case "update-now": On 2016/02/01 18:52:36, Thomas Greiner wrote: > Detail: For consistency I'd suggest renaming this to "update-subscription". Also > because "update-now" could be confused with "update-all-subscriptions". Fare enough, done. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:620: body[data-tab="advanced-allFilterLists"] #content-advanced [data-tab="advanced-allFilterLists"], On 2016/02/01 18:52:37, Thomas Greiner wrote: > Detail: Mentioning `#content-advanced` is redundant since the tab name already > mentions that it's part of the Advanced tab. Done. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:690: position: relative; On 2016/02/01 18:52:37, Thomas Greiner wrote: > Detail: According to the coding style positioning should come before box > model-related properties. Done. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:694: margin: 0px 4px; On 2016/02/01 18:52:37, Thomas Greiner wrote: > The reason why it looks odd to you to specify `6px` is because there's > whitespace between the subscription title and the arrow. Therefore using `4px` > instead is merely a workaround for the underlying issue. > > It seems that specifying `display: flex` on the parent and `vertical-align: > middle; margin: auto 6px;` resolves the issue and properly align it vertically. Done, also changed the parent to be block level element, not to have nested block level element in the inline one. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:695: background-color: transparent; On 2016/02/01 18:52:38, Thomas Greiner wrote: > Detail: Why is that necessary? Redundant, do not remember why I have it here. Done. Thanks for noticing. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:968: border-radius: 3px; On 2016/02/01 18:52:37, Thomas Greiner wrote: > Detail: Why did you move the border radius to here? I think I didn't noticed that. moved to content. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:982: div.context-menu .content On 2016/02/01 18:52:38, Thomas Greiner wrote: > Detail: Please add `cursor: default` to avoid showing the pointing cursor when > hovering over non-clickable elements in the context menu. Done. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:1011: text-decoration: none; On 2016/02/01 18:52:36, Thomas Greiner wrote: > Detail: This property is in the wrong spot according to our coding style since > it belongs into the "Colors and Typography" category. Done. https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:1018: div.context-menu > div a:before On 2016/02/01 18:52:38, Thomas Greiner wrote: > Detail: Replace ":before" with "::before" to make it consistent with the rest of > the code. Done.
https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:620: body[data-tab="advanced-allFilterLists"] #content-advanced [data-tab="advanced-allFilterLists"], On 2016/02/03 14:04:29, saroyanm wrote: > On 2016/02/01 18:52:37, Thomas Greiner wrote: > > Detail: Mentioning `#content-advanced` is redundant since the tab name already > > mentions that it's part of the Advanced tab. > > Done. I noticed that this change introduces a regression: Try clicking on the tabs in the sidebar and you'll notice that the "Advanced" tab's proportions change. So it seems like we need to revert that change, unfortunately. https://codereview.adblockplus.org/29333819/diff/29335489/background.js File background.js (right): https://codereview.adblockplus.org/29333819/diff/29335489/background.js#newco... background.js:78: this.downloadStatus = params.downloadStatus; Please document those newly introduced parameters in README.md https://codereview.adblockplus.org/29333819/diff/29335489/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29335489/options.js#newcode191 options.js:191: timeElement.textContent = getMessage("options_filterList_lastDownload_inProgress"); Whenever adding a subscription I see two issues: 1) The text "Downloading…" stays there forever 2) The date and time icons are still shown (same when an error message shows up) https://codereview.adblockplus.org/29333819/diff/29335489/options.js#newcode386 options.js:386: this["$" + property] = newValue; Detail: Shouldn't this only be set whenever the value changed?
https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29335133/skin/options.css#ne... skin/options.css:620: body[data-tab="advanced-allFilterLists"] #content-advanced [data-tab="advanced-allFilterLists"], On 2016/02/03 14:50:13, Thomas Greiner wrote: > On 2016/02/03 14:04:29, saroyanm wrote: > > On 2016/02/01 18:52:37, Thomas Greiner wrote: > > > Detail: Mentioning `#content-advanced` is redundant since the tab name > already > > > mentions that it's part of the Advanced tab. > > > > Done. > > I noticed that this change introduces a regression: Try clicking on the tabs in > the sidebar and you'll notice that the "Advanced" tab's proportions change. > > So it seems like we need to revert that change, unfortunately. Ach right, because we also specify same data-tab both on sidebar tabs and the one in advanced, thanks for noticing that. https://codereview.adblockplus.org/29333819/diff/29335489/background.js File background.js (right): https://codereview.adblockplus.org/29333819/diff/29335489/background.js#newco... background.js:78: this.downloadStatus = params.downloadStatus; On 2016/02/03 14:50:14, Thomas Greiner wrote: > Please document those newly introduced parameters in README.md Done. https://codereview.adblockplus.org/29333819/diff/29335489/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29335489/options.js#newcode191 options.js:191: timeElement.textContent = getMessage("options_filterList_lastDownload_inProgress"); On 2016/02/03 14:50:14, Thomas Greiner wrote: > Whenever adding a subscription I see two issues: > > 1) The text "Downloading…" stays there forever > 2) The date and time icons are still shown (same when an error message shows up) Done. https://codereview.adblockplus.org/29333819/diff/29335489/options.js#newcode386 options.js:386: this["$" + property] = newValue; On 2016/02/03 14:50:14, Thomas Greiner wrote: > Detail: Shouldn't this only be set whenever the value changed? Done.
https://codereview.adblockplus.org/29333819/diff/29335515/background.js File background.js (right): https://codereview.adblockplus.org/29333819/diff/29335515/background.js#newco... background.js:316: knownSubscriptions[subscriptionUrl].lastDownload = 1234; This will trigger the "subscriptions.lastDownload" message which is not supposed to happen. For initialization the extension uses `Subscription.fromObject()` but since this is just a mock implementation it should be sufficient to set "_lastDownload" here. https://codereview.adblockplus.org/29333819/diff/29335515/options.html File options.html (right): https://codereview.adblockplus.org/29333819/diff/29335515/options.html#newcod... options.html:261: <span class="progress">progress</span> We don't intend to show both at the same time so what about replacing them with a single `<span class="message">`? That should also make the logic in options.js and the styles in options.css a bit simpler. https://codereview.adblockplus.org/29333819/diff/29335515/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29335515/options.js#newcode194 options.js:194: var text = getMessage("options_filterList_lastDownload_inProgress"); Interesting that you would set the text only when "lastDownload" is `0` since that is only the case for subscriptions that haven't been downloaded before. Please check how it is implemented in the current options page because this approach seems to require us to work around it by setting "lastDownload" to `0` explicitly which never occurs in the extension. https://codereview.adblockplus.org/29333819/diff/29335515/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29335515/skin/options.css#ne... skin/options.css:720: #all-filter-lists li.error .error `.error` and `.progress` are not very descriptive of the element's state especially since you're using them for two different elements here so I'd suggest using something like `.show-message` instead. https://codereview.adblockplus.org/29333819/diff/29335515/skin/options.css#ne... skin/options.css:722: display: inline; Detail: You can simplify this a bit by merging these two blocks by writing it as: #all-filter-lists li.show-message .date, #all-filter-lists li.show-message .time, #all-filter-lists li:not(.show-message) .message { display: none; }
https://codereview.adblockplus.org/29333819/diff/29335515/background.js File background.js (right): https://codereview.adblockplus.org/29333819/diff/29335515/background.js#newco... background.js:316: knownSubscriptions[subscriptionUrl].lastDownload = 1234; On 2016/02/04 15:28:16, Thomas Greiner wrote: > This will trigger the "subscriptions.lastDownload" message which is not supposed > to happen. For initialization the extension uses `Subscription.fromObject()` but > since this is just a mock implementation it should be sufficient to set > "_lastDownload" here. Done. https://codereview.adblockplus.org/29333819/diff/29335515/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29335515/options.js#newcode194 options.js:194: var text = getMessage("options_filterList_lastDownload_inProgress"); On 2016/02/04 15:28:17, Thomas Greiner wrote: > Interesting that you would set the text only when "lastDownload" is `0` since > that is only the case for subscriptions that haven't been downloaded before. > Please check how it is implemented in the current options page because this > approach seems to require us to work around it by setting "lastDownload" to `0` > explicitly which never occurs in the extension. You are right, I've updated the implementation, Also tested on production env. looks fine, the only problem is that the background mock implementation doesn't look too nice, but I assume it's fine while it's just a mock ? https://codereview.adblockplus.org/29333819/diff/29335515/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29333819/diff/29335515/skin/options.css#ne... skin/options.css:720: #all-filter-lists li.error .error On 2016/02/04 15:28:17, Thomas Greiner wrote: > `.error` and `.progress` are not very descriptive of the element's state > especially since you're using them for two different elements here so I'd > suggest using something like `.show-message` instead. Done. https://codereview.adblockplus.org/29333819/diff/29335515/skin/options.css#ne... skin/options.css:722: display: inline; On 2016/02/04 15:28:17, Thomas Greiner wrote: > Detail: You can simplify this a bit by merging these two blocks by writing it > as: > > #all-filter-lists li.show-message .date, > #all-filter-lists li.show-message .time, > #all-filter-lists li:not(.show-message) .message > { > display: none; > } Done.
Sorry about this lengthy review, LGTM Just some remarks after skimming through the overall diff. https://codereview.adblockplus.org/29333819/diff/29335515/options.js File options.js (right): https://codereview.adblockplus.org/29333819/diff/29335515/options.js#newcode194 options.js:194: var text = getMessage("options_filterList_lastDownload_inProgress"); On 2016/02/04 17:50:06, saroyanm wrote: > On 2016/02/04 15:28:17, Thomas Greiner wrote: > > Interesting that you would set the text only when "lastDownload" is `0` since > > that is only the case for subscriptions that haven't been downloaded before. > > Please check how it is implemented in the current options page because this > > approach seems to require us to work around it by setting "lastDownload" to > `0` > > explicitly which never occurs in the extension. > > You are right, I've updated the implementation, > Also tested on production env. looks fine, the only problem is that the > background mock implementation doesn't look too nice, but I assume it's fine > while it's just a mock ? Agreed, let's leave it for now if it's merely a minor cosmetic flaw. https://codereview.adblockplus.org/29333819/diff/29335640/background.js File background.js (right): https://codereview.adblockplus.org/29333819/diff/29335640/background.js#newco... background.js:207: downloading: false, Detail: Since this is a "private" property I'd suggest renaming it to "_downloading". https://codereview.adblockplus.org/29333819/diff/29335640/background.js#newco... background.js:322: Detail: This empty line has been added without touching any other part of the code in this area so better revert it. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
