Issue 29445590: Issue 5255 - Advanced tab (HTML, strings and functionality) (Closed)
|
Created:
May 22, 2017, 10:25 a.m. by saroyanm Modified:
July 17, 2017, 8:49 a.m. Reviewers:
Thomas Greiner CC:
wspee, juliandoucette Visibility:
Public. |
DescriptionThis review contains HTML/Semantics, strings and functional implementation excluding Dialog implementations of advanced tab according to the specifications ( https://bitbucket.org/adblockplus/spec/src/04b3af8ed03c01a5cc36f575f3c2865559731ee0/spec/abp/options-page.md?at=master&fileviewer=file-view-default#markdown-header-advanced-tab ).
Patch Set 1 : HTML, Strings and functionality #
Total comments: 27
Patch Set 2 : Addressed comments from the review meeting #
Total comments: 6
Patch Set 3 : Addressed notes from weekly(ish) meeting and small collection fix #
Total comments: 53
Patch Set 4 : Addressed Thomas comments #
Total comments: 23
Patch Set 5 : Rebased to Changeset #111 #Patch Set 6 : Removed unnecessary function call #
Total comments: 1
Patch Set 7 : Addressed Thomas comments #
Total comments: 4
Patch Set 8 : Fixed nits #
MessagesTotal messages: 17
Ready for the review. https://codereview.adblockplus.org/29445590/diff/29462576/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29445590/diff/29462576/locale/en-US/new-op... locale/en-US/new-options.json:204: "message": "Each Adblock Plus setting functions because of a filter list. Below are the corresponding filter lists to all your Adblock Plus settings. You can also add additional filters created and maintained by our trusted community. <a>Learn more</a>." We do parse a and strong tags here -> https://hg.adblockplus.org/adblockplusui/file/tip/i18n.js#l47 But I can't see way to set attributes. Am I missing something, or we do not have an implementation in place yet ? https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html File new-options.html (left): https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html#ol... new-options.html:267: <span class="date"></span> We do have date and time still in the sprite: * I'm not sure if we will continue using sprites (in case we will use SVGs). So if we will remove this icons from the sprites it will require adjusting other icons as well (because of position change in the sprite). So I'm not sure if the effort worth it rather than handling the sprite changes in separate review. I lean towards handling that it separate review. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html#ne... new-options.html:212: <span class="i18n_options_readMore" I think redoing the tool-tips belong to another review. IMO: we can reuse the old text and implementation for current review yet. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html#ne... new-options.html:240: <ul class="table cols" id="all-filter-lists-table"> Shouldn't this be <table> rather than list ? It looks more like tabular data, than list ? Or at least we should find way of linking the header to column, maybe "labelledby" ? https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html#ne... new-options.html:259: <div data-action="open-context-menu" class="arrow"> This should be button instead: Focusing button looks bit ugly in this case, might make sense to find a solution in current review. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js#newc... new-options.js:237: let monthName = lastUpdateDate.toLocaleString("en-US", For reference: Make sense to find a way to pass the Language code references to here and also to HTML Doc. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js#newc... new-options.js:427: // Note: document.createElement("option") is unreliable in Opera I'm not sure if this still true, I took this from current(old) options page implementation. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js#newc... new-options.js:611: sendMessageHandleErrors({ I still need to figure out how to get exact filters that cause errors, while this is not implemented for the test server I'll check the actual implementation and update accordingly. (We can also make that a separate review not to block current one)
Notes from the review meeting. https://codereview.adblockplus.org/29445590/diff/29462576/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29445590/diff/29462576/locale/en-US/new-op... locale/en-US/new-options.json:204: "message": "Each Adblock Plus setting functions because of a filter list. Below are the corresponding filter lists to all your Adblock Plus settings. You can also add additional filters created and maintained by our trusted community. <a>Learn more</a>." On 2017/06/11 15:40:11, saroyanm wrote: > We do parse a and strong tags here -> > https://hg.adblockplus.org/adblockplusui/file/tip/i18n.js#l47 > But I can't see way to set attributes. > > Am I missing something, or we do not have an implementation in place yet ? We do have implementation in FirstRun page, use that as an example. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html File new-options.html (left): https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html#ol... new-options.html:267: <span class="date"></span> On 2017/06/11 15:40:11, saroyanm wrote: > We do have date and time still in the sprite: > * I'm not sure if we will continue using sprites (in case we will use SVGs). > So if we will remove this icons from the sprites it will require adjusting other > icons as well (because of position change in the sprite). So I'm not sure if the > effort worth it rather than handling the sprite changes in separate review. > I lean towards handling that it separate review. I will remove the icon from the sprite, but leave the background. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html#ne... new-options.html:212: <span class="i18n_options_readMore" On 2017/06/11 15:40:11, saroyanm wrote: > I think redoing the tool-tips belong to another review. > IMO: we can reuse the old text and implementation for current review yet. Acknowledged. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html#ne... new-options.html:240: <ul class="table cols" id="all-filter-lists-table"> On 2017/06/11 15:40:11, saroyanm wrote: > Shouldn't this be <table> rather than list ? > It looks more like tabular data, than list ? > Or at least we should find way of linking the header to column, maybe > "labelledby" ? If it easy to implement add support for both table and UL, if it's not keep using current solution. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html#ne... new-options.html:259: <div data-action="open-context-menu" class="arrow"> On 2017/06/11 15:40:11, saroyanm wrote: > This should be button instead: > Focusing button looks bit ugly in this case, might make sense to find a solution > in current review. Use <a> tag instead of DIV https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js#newc... new-options.js:237: let monthName = lastUpdateDate.toLocaleString("en-US", On 2017/06/11 15:40:12, saroyanm wrote: > For reference: Make sense to find a way to pass the Language code references to > here and also to HTML Doc. Create a function in i18n, that returns month: "short" according to the passed parameters: ex.: in i18n.js: function toLocaleString(date, options) {return date.toLocaleString(locale, options);} https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js#newc... new-options.js:241: lastUpdateElem.textContent = day + " " + monthName + " " + Note: Make sense to discuss if we need to localize "DD MMM YYYY" format. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js#newc... new-options.js:427: // Note: document.createElement("option") is unreliable in Opera On 2017/06/11 15:40:12, saroyanm wrote: > I'm not sure if this still true, I took this from current(old) options page > implementation. Check if it's still an issue in the supported Opera versions. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js#newc... new-options.js:611: sendMessageHandleErrors({ On 2017/06/11 15:40:12, saroyanm wrote: > I still need to figure out how to get exact filters that cause errors, while > this is not implemented for the test server I'll check the actual implementation > and update accordingly. > (We can also make that a separate review not to block current one) Separate this into another review.
https://codereview.adblockplus.org/29445590/diff/29462576/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29445590/diff/29462576/locale/en-US/new-op... locale/en-US/new-options.json:204: "message": "Each Adblock Plus setting functions because of a filter list. Below are the corresponding filter lists to all your Adblock Plus settings. You can also add additional filters created and maintained by our trusted community. <a>Learn more</a>." On 2017/06/12 16:44:41, saroyanm wrote: > On 2017/06/11 15:40:11, saroyanm wrote: > > We do parse a and strong tags here -> > > https://hg.adblockplus.org/adblockplusui/file/tip/i18n.js#l47 > > But I can't see way to set attributes. > > > > Am I missing something, or we do not have an implementation in place yet ? > > We do have implementation in FirstRun page, use that as an example. Done. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html File new-options.html (left): https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html#ol... new-options.html:267: <span class="date"></span> On 2017/06/12 16:44:42, saroyanm wrote: > On 2017/06/11 15:40:11, saroyanm wrote: > > We do have date and time still in the sprite: > > * I'm not sure if we will continue using sprites (in case we will use SVGs). > > So if we will remove this icons from the sprites it will require adjusting > other > > icons as well (because of position change in the sprite). So I'm not sure if > the > > effort worth it rather than handling the sprite changes in separate review. > > I lean towards handling that it separate review. > > I will remove the icon from the sprite, but leave the background. Done. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html#ne... new-options.html:240: <ul class="table cols" id="all-filter-lists-table"> On 2017/06/12 16:44:41, saroyanm wrote: > On 2017/06/11 15:40:11, saroyanm wrote: > > Shouldn't this be <table> rather than list ? > > It looks more like tabular data, than list ? > > Or at least we should find way of linking the header to column, maybe > > "labelledby" ? > > If it easy to implement add support for both table and UL, if it's not keep > using current solution. After investigation, it appears that it might require bit of effort/test and additional review, so I created a separate issue for that -> https://issues.adblockplus.org/ticket/5317 IMO opinion we should first implement what is crucial for launch and fix issues like this separately, hopefully before the launch, if not before, we definitely should fix them as second priority after the priorities for the launch. I wish to fix this ASAP, but don't want to make blocker to current review. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html#ne... new-options.html:259: <div data-action="open-context-menu" class="arrow"> On 2017/06/12 16:44:42, saroyanm wrote: > On 2017/06/11 15:40:11, saroyanm wrote: > > This should be button instead: > > Focusing button looks bit ugly in this case, might make sense to find a > solution > > in current review. > > Use <a> tag instead of DIV Done. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js#newc... new-options.js:237: let monthName = lastUpdateDate.toLocaleString("en-US", On 2017/06/12 16:44:42, saroyanm wrote: > On 2017/06/11 15:40:12, saroyanm wrote: > > For reference: Make sense to find a way to pass the Language code references > to > > here and also to HTML Doc. > > Create a function in i18n, that returns month: "short" according to the passed > parameters: > ex.: in i18n.js: function toLocaleString(date, options) {return > date.toLocaleString(locale, options);} I used different approach while we do have the value set for the HTML doc, so we can refer to DOM API to retrieve it. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js#newc... new-options.js:427: // Note: document.createElement("option") is unreliable in Opera On 2017/06/12 16:44:42, saroyanm wrote: > On 2017/06/11 15:40:12, saroyanm wrote: > > I'm not sure if this still true, I took this from current(old) options page > > implementation. > > Check if it's still an issue in the supported Opera versions. Done. This was added back in 2012 -> https://hg.adblockplus.org/adblockpluschrome/rev/0fca6c3f3462#l1.62 Anyway I don't see benefit of using document.createElement as we can pass text and value into the constructor of Option() API and I think the comment itself is redundant.
Added notes from the weekly meeting, that are still missing, will update the new patch fixing them today. https://codereview.adblockplus.org/29445590/diff/29465568/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29445590/diff/29465568/new-options.html#ne... new-options.html:271: <li class="static"> We shouldn't list Own filter list in the filter lists table anymore. see -> https://bitbucket.org/adblockplus/spec/issues/28/ https://codereview.adblockplus.org/29445590/diff/29465568/new-options.html#ne... new-options.html:304: <textarea id="custom-filters-raw"></textarea> The placeholder is missing. https://codereview.adblockplus.org/29445590/diff/29465568/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29465568/new-options.js#newc... new-options.js:132: if (table.hasChildNodes()) This should be "table.children" so we can ignore text nodes.
New patch uploaded. Ready for the review. https://codereview.adblockplus.org/29445590/diff/29465568/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29445590/diff/29465568/new-options.html#ne... new-options.html:271: <li class="static"> On 2017/06/14 11:07:06, saroyanm wrote: > We shouldn't list Own filter list in the filter lists table anymore. see -> > https://bitbucket.org/adblockplus/spec/issues/28/ Done. https://codereview.adblockplus.org/29445590/diff/29465568/new-options.html#ne... new-options.html:304: <textarea id="custom-filters-raw"></textarea> On 2017/06/14 11:07:06, saroyanm wrote: > The placeholder is missing. Done. https://codereview.adblockplus.org/29445590/diff/29465568/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29465568/new-options.js#newc... new-options.js:132: if (table.hasChildNodes()) On 2017/06/14 11:07:06, saroyanm wrote: > This should be "table.children" so we can ignore text nodes. Done.
https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29445590/diff/29462576/new-options.html#ne... new-options.html:240: <ul class="table cols" id="all-filter-lists-table"> On 2017/06/14 10:59:00, saroyanm wrote: > On 2017/06/12 16:44:41, saroyanm wrote: > > On 2017/06/11 15:40:11, saroyanm wrote: > > > Shouldn't this be <table> rather than list ? > > > It looks more like tabular data, than list ? > > > Or at least we should find way of linking the header to column, maybe > > > "labelledby" ? > > > > If it easy to implement add support for both table and UL, if it's not keep > > using current solution. > > After investigation, it appears that it might require bit of effort/test and > additional review, so I created a separate issue for that -> > https://issues.adblockplus.org/ticket/5317 > IMO opinion we should first implement what is crucial for launch and fix issues > like this separately, hopefully before the launch, if not before, we definitely > should fix them as second priority after the priorities for the launch. > I wish to fix this ASAP, but don't want to make blocker to current review. Acknowledged. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js#newc... new-options.js:237: let monthName = lastUpdateDate.toLocaleString("en-US", On 2017/06/14 10:59:00, saroyanm wrote: > On 2017/06/12 16:44:42, saroyanm wrote: > > On 2017/06/11 15:40:12, saroyanm wrote: > > > For reference: Make sense to find a way to pass the Language code references > > to > > > here and also to HTML Doc. > > > > Create a function in i18n, that returns month: "short" according to the passed > > parameters: > > ex.: in i18n.js: function toLocaleString(date, options) {return > > date.toLocaleString(locale, options);} > > I used different approach while we do have the value set for the HTML doc, so we > can refer to DOM API to retrieve it. This looks like a race condition because the "lang" attribute might not have been set at this point yet. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js#newc... new-options.js:427: // Note: document.createElement("option") is unreliable in Opera On 2017/06/14 10:59:00, saroyanm wrote: > On 2017/06/12 16:44:42, saroyanm wrote: > > On 2017/06/11 15:40:12, saroyanm wrote: > > > I'm not sure if this still true, I took this from current(old) options page > > > implementation. > > > > Check if it's still an issue in the supported Opera versions. > > Done. > > This was added back in 2012 -> > https://hg.adblockplus.org/adblockpluschrome/rev/0fca6c3f3462#l1.62 > > Anyway I don't see benefit of using document.createElement as we can pass text > and value into the constructor of Option() API and I think the comment itself is > redundant. Acknowledged. https://codereview.adblockplus.org/29445590/diff/29462576/new-options.js#newc... new-options.js:611: sendMessageHandleErrors({ On 2017/06/12 16:44:42, saroyanm wrote: > On 2017/06/11 15:40:12, saroyanm wrote: > > I still need to figure out how to get exact filters that cause errors, while > > this is not implemented for the test server I'll check the actual > implementation > > and update accordingly. > > (We can also make that a separate review not to block current one) > > Separate this into another review. Acknowledged. https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... locale/en-US/new-options.json:172: "message": "CUSTOMIZE ON-PAGE ACTIONS" Detail: Don't capitalize the text here or otherwise we'd have to redo the translation whenever we decide to capitalize the text differently. Instead use `text-transform: uppercase;` in the CSS. Same applies to "options_customFilters_title". https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... locale/en-US/new-options.json:226: "options_filterList_just": { Detail: "just" is not a time indicator. Instead I'd suggest using "now". https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... locale/en-US/new-options.json:232: "message": "minutes ago" These two texts don't contain placeholders for the number of minutes/hours. In some languages the number may appear not just before or after but in the middle of the text. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29445590/diff/29465584/new-options.html#ne... new-options.html:260: <div role="menubar" class="context-menu-wrapper"> Since we're now making the context menu accessible, it'd be great if you could use `<ul>` and `<li>` instead of `<div>` as seen on https://www.w3.org/WAI/GL/wiki/Using_ARIA_menus#Example_Code. Note that later on we should also implement proper focus handling but no need to tackle that one as part of this review. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.html#ne... new-options.html:297: <select id="custom-filters-box" multiple></select> What's the reason for making this a `<select>` element? Unlike the current options page we don't allow the user to select individual filters to remove them. If you want to keep things simple you could just use `<textarea disabled>` and style it via the `:disabled` pseudo class. That way you may be able to completely get rid of `addCustomFilter()`, `removeCustomFilter()` and `convertToRawFormat()`. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:136: } What's the reason for those changes? I didn't notice any header being added. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:229: let timer = setInterval(lastUpdateLive, 60000); As far as I see, the spec doesn't say that this time is supposed to be kept up-to-date while the page is open. Furthermore, here are some optional improvements in case it turns out that this feature is required: - No need to update it every minute if `lastUpdate > 1 hour` - The next minute might be reached even before the 60 seconds have passed since it's unlikely that we set the interval at exactly zero seconds. - The interval seems to continue even after the item gets removed. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:234: if(sinceUpdate > 86400000) Detail: Usually, we create constants to make it more obvious what all those different numbers are supposed to represent. See - https://hg.adblockplus.org/adblockpluscore/file/tip/lib/synchronizer.js#l33 - https://hg.adblockplus.org/adblockpluscore/file/tip/lib/downloader.js#l25 https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:238: document.documentElement.lang, { month: "short" }); Actually, while reviewing #5278 I noticed that we could simply pass `undefined` for the locale value to use the current value. If you remember we did try passing `null` and omitting the first parameter to achieve that but neither one worked. On another note, we could even go as far as to use new Intl.DateTimeFormat(undefined, {year: "numeric", month: "short", day: "2-digit"}).format(new Date()) instead to format the date but it doesn't seem like we could get the format "DD MMM YYYY" out of it so it doesn't seem too useful for what we need here. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:238: document.documentElement.lang, { month: "short" }); Coding style: This violates the rule "object-curly-spacing" from eslint-config-eyeo. See - https://hg.adblockplus.org/codingtools/file/tip/eslint-config-eyeo/index.js#l84 - http://eslint.org/docs/rules/object-curly-spacing https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:425: customFiltersView("read"); Detail: The name is ambiguous because it doesn't say what happens with the custom filters view. Names such as "setCustomFiltersView" or "switchCustomFiltersView" would clarify that. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:537: if (element.getAttribute("href") == "#") Detail: Why do you even set the "href" attribute in the first place? https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:663: function customFiltersView(mode) You're trying to switch between three modes but each class can only represent two values. The simplest solutions for this scenario would be to either not use classes (e.g. `data-mode="view"`) or to use two classes (e.g. `.empty.view`). https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:663: function customFiltersView(mode) Detail: I noticed that you've call `E("custom-filters")` quite often and unconditionally so let's introduce a variable for that. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:674: E("custom-filters").classList.add("mode-view"); Detail: Why the different naming? I do like that you chose to go with "read" and "write" instead of "view" and "edit" but why not use them consistently throughout? https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:676: E("custom-filters").classList.remove("mode-empty"); Detail: Note that you can pass multiple arguments to `HTMLElement.classList.add()` and `HTMLElement.classList.remove()` so no need to call them multiple times. https://codereview.adblockplus.org/29445590/diff/29465584/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29445590/diff/29465584/skin/new-options.cs... skin/new-options.css:699: #all-filter-lists-table .static label Detail: You removed the only element with the "static" class so it seems that we can remove this now.
Replied to your comments, 2 of them I'd like to discuss with you before starting implementing next patch. https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... locale/en-US/new-options.json:172: "message": "CUSTOMIZE ON-PAGE ACTIONS" On 2017/07/07 13:01:08, Thomas Greiner wrote: > Detail: Don't capitalize the text here or otherwise we'd have to redo the > translation whenever we decide to capitalize the text differently. Instead use > `text-transform: uppercase;` in the CSS. > > Same applies to "options_customFilters_title". My idea was to capitalize and rollback to the lower case after introduction of CSS UPPER_CASE, but while you mentioned this, I'll follow your suggestion. https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... locale/en-US/new-options.json:226: "options_filterList_just": { On 2017/07/07 13:01:08, Thomas Greiner wrote: > Detail: "just" is not a time indicator. Instead I'd suggest using "now". Agree https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... locale/en-US/new-options.json:232: "message": "minutes ago" On 2017/07/07 13:01:08, Thomas Greiner wrote: > These two texts don't contain placeholders for the number of minutes/hours. In > some languages the number may appear not just before or after but in the middle > of the text. I agree, I'll introduce placeholder here. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29445590/diff/29465584/new-options.html#ne... new-options.html:260: <div role="menubar" class="context-menu-wrapper"> On 2017/07/07 13:01:09, Thomas Greiner wrote: > Since we're now making the context menu accessible, it'd be great if you could > use `<ul>` and `<li>` instead of `<div>` as seen on > https://www.w3.org/WAI/GL/wiki/Using_ARIA_menus#Example_Code. > > Note that later on we should also implement proper focus handling but no need to > tackle that one as part of this review. Good point. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.html#ne... new-options.html:297: <select id="custom-filters-box" multiple></select> On 2017/07/07 13:01:09, Thomas Greiner wrote: > What's the reason for making this a `<select>` element? Unlike the current > options page we don't allow the user to select individual filters to remove > them. > > If you want to keep things simple you could just use `<textarea disabled>` and > style it via the `:disabled` pseudo class. That way you may be able to > completely get rid of `addCustomFilter()`, `removeCustomFilter()` and > `convertToRawFormat()`. Interesting, shouldn't we allow them to delete the filters, otherwise this will be a regression, what you think? Anyway I agree that your suggestion is more simple. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:136: } On 2017/07/07 13:01:09, Thomas Greiner wrote: > What's the reason for those changes? I didn't notice any header being added. Header is column name. I used naming header from "<TH>" element. beforeIndex is increased by one in case table have header column. While this is specified in the HTML rather than generated dynamically it's easy to use this solution. Anyway let's discuss this implementation. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:229: let timer = setInterval(lastUpdateLive, 60000); On 2017/07/07 13:01:09, Thomas Greiner wrote: > As far as I see, the spec doesn't say that this time is supposed to be kept > up-to-date while the page is open. I do remember that this was discussed during our meetings and the outcome was to update this live. Anyway this should end up in the specs as well, if it's not yet. I'll take care of it. > Furthermore, here are some optional improvements in case it turns out that this > feature is required: > - No need to update it every minute if `lastUpdate > 1 hour` agree > - The next minute might be reached even before the 60 seconds have passed since agree > it's unlikely that we set the interval at exactly zero seconds. > - The interval seems to continue even after the item gets removed. Good point, I'll remove it. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:234: if(sinceUpdate > 86400000) On 2017/07/07 13:01:10, Thomas Greiner wrote: > Detail: Usually, we create constants to make it more obvious what all those > different numbers are supposed to represent. > > See > - https://hg.adblockplus.org/adblockpluscore/file/tip/lib/synchronizer.js#l33 > - https://hg.adblockplus.org/adblockpluscore/file/tip/lib/downloader.js#l25 Agree https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:238: document.documentElement.lang, { month: "short" }); On 2017/07/07 13:01:10, Thomas Greiner wrote: > Coding style: This violates the rule "object-curly-spacing" from > eslint-config-eyeo. > > See > - > https://hg.adblockplus.org/codingtools/file/tip/eslint-config-eyeo/index.js#l84 > - http://eslint.org/docs/rules/object-curly-spacing Good point, I'll run eslint before uploading next patch. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:238: document.documentElement.lang, { month: "short" }); On 2017/07/07 13:01:10, Thomas Greiner wrote: > Actually, while reviewing #5278 I noticed that we could simply pass `undefined` > for the locale value to use the current value. If you remember we did try > passing `null` and omitting the first parameter to achieve that but neither one > worked. Acknowledged, checked specifications as well I think we good to go with undefined. Undefined suppose to the value if "locale" parameter is missing. > On another note, we could even go as far as to use > > new Intl.DateTimeFormat(undefined, {year: "numeric", month: "short", day: > "2-digit"}).format(new Date()) > > instead to format the date but it doesn't seem like we could get the format "DD > MMM YYYY" out of it so it doesn't seem too useful for what we need here. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:425: customFiltersView("read"); On 2017/07/07 13:01:10, Thomas Greiner wrote: > Detail: The name is ambiguous because it doesn't say what happens with the > custom filters view. Names such as "setCustomFiltersView" or > "switchCustomFiltersView" would clarify that. Agree. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:537: if (element.getAttribute("href") == "#") On 2017/07/07 13:01:10, Thomas Greiner wrote: > Detail: Why do you even set the "href" attribute in the first place? Good question, I think we don't need href attribute, at all, I thought it's some kind of common practice to specify href attribute, but I think we can remove it. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:663: function customFiltersView(mode) On 2017/07/07 13:01:09, Thomas Greiner wrote: > Detail: I noticed that you've call `E("custom-filters")` quite often and > unconditionally so let's introduce a variable for that. Agree. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:663: function customFiltersView(mode) On 2017/07/07 13:01:09, Thomas Greiner wrote: > You're trying to switch between three modes but each class can only represent > two values. The simplest solutions for this scenario would be to either not use > classes (e.g. `data-mode="view"`) or to use two classes (e.g. `.empty.view`). ".mode-edit" Will be consistent with other implementation we already introduced, previously. But I agree that data-mode="view" is more clear implementation. I think that we are safe to switch to using data attributes, that will be consistent with how we are switching between tabs. And inconsistency between using class attribute will be removed after #5391 is resolved. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:674: E("custom-filters").classList.add("mode-view"); On 2017/07/07 13:01:10, Thomas Greiner wrote: > Detail: Why the different naming? I do like that you chose to go with "read" and > "write" instead of "view" and "edit" but why not use them consistently > throughout? Acknowledged, will make them consistent https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:676: E("custom-filters").classList.remove("mode-empty"); On 2017/07/07 13:01:09, Thomas Greiner wrote: > Detail: Note that you can pass multiple arguments to > `HTMLElement.classList.add()` and `HTMLElement.classList.remove()` so no need to > call them multiple times. Acknowledged. https://codereview.adblockplus.org/29445590/diff/29465584/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29445590/diff/29465584/skin/new-options.cs... skin/new-options.css:699: #all-filter-lists-table .static label On 2017/07/07 13:01:11, Thomas Greiner wrote: > Detail: You removed the only element with the "static" class so it seems that we > can remove this now. Well spotted.
Notes from the meeting. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29445590/diff/29465584/new-options.html#ne... new-options.html:297: <select id="custom-filters-box" multiple></select> On 2017/07/10 11:38:00, saroyanm wrote: > On 2017/07/07 13:01:09, Thomas Greiner wrote: > > What's the reason for making this a `<select>` element? Unlike the current > > options page we don't allow the user to select individual filters to remove > > them. > > > > If you want to keep things simple you could just use `<textarea disabled>` and > > style it via the `:disabled` pseudo class. That way you may be able to > > completely get rid of `addCustomFilter()`, `removeCustomFilter()` and > > `convertToRawFormat()`. > > Interesting, shouldn't we allow them to delete the filters, otherwise this will > be a regression, what you think? Anyway I agree that your suggestion is more > simple. This will not be a regression, as user still can remove the filter from the textarea. Follow Thomas suggestion regarding implementation. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:136: } On 2017/07/10 11:38:00, saroyanm wrote: > On 2017/07/07 13:01:09, Thomas Greiner wrote: > > What's the reason for those changes? I didn't notice any header being added. > > Header is column name. I used naming header from "<TH>" element. > beforeIndex is increased by one in case table have header column. > > While this is specified in the HTML rather than generated dynamically it's easy > to use this solution. Anyway let's discuss this implementation. I'll move the column names row out of the list for current review.
New patch uploaded. https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... locale/en-US/new-options.json:172: "message": "CUSTOMIZE ON-PAGE ACTIONS" On 2017/07/10 11:37:59, saroyanm wrote: > On 2017/07/07 13:01:08, Thomas Greiner wrote: > > Detail: Don't capitalize the text here or otherwise we'd have to redo the > > translation whenever we decide to capitalize the text differently. Instead use > > `text-transform: uppercase;` in the CSS. > > > > Same applies to "options_customFilters_title". > > My idea was to capitalize and rollback to the lower case after introduction of > CSS UPPER_CASE, but while you mentioned this, I'll follow your suggestion. Done. https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... locale/en-US/new-options.json:226: "options_filterList_just": { On 2017/07/10 11:37:59, saroyanm wrote: > On 2017/07/07 13:01:08, Thomas Greiner wrote: > > Detail: "just" is not a time indicator. Instead I'd suggest using "now". > > Agree Done. https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... locale/en-US/new-options.json:232: "message": "minutes ago" On 2017/07/10 11:37:59, saroyanm wrote: > On 2017/07/07 13:01:08, Thomas Greiner wrote: > > These two texts don't contain placeholders for the number of minutes/hours. In > > some languages the number may appear not just before or after but in the > middle > > of the text. > > I agree, I'll introduce placeholder here. Done. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29445590/diff/29465584/new-options.html#ne... new-options.html:260: <div role="menubar" class="context-menu-wrapper"> On 2017/07/10 11:38:00, saroyanm wrote: > On 2017/07/07 13:01:09, Thomas Greiner wrote: > > Since we're now making the context menu accessible, it'd be great if you could > > use `<ul>` and `<li>` instead of `<div>` as seen on > > https://www.w3.org/WAI/GL/wiki/Using_ARIA_menus#Example_Code. > > > > Note that later on we should also implement proper focus handling but no need > to > > tackle that one as part of this review. > > Good point. Done. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.html#ne... new-options.html:297: <select id="custom-filters-box" multiple></select> On 2017/07/10 13:27:43, saroyanm wrote: > On 2017/07/10 11:38:00, saroyanm wrote: > > On 2017/07/07 13:01:09, Thomas Greiner wrote: > > > What's the reason for making this a `<select>` element? Unlike the current > > > options page we don't allow the user to select individual filters to remove > > > them. > > > > > > If you want to keep things simple you could just use `<textarea disabled>` > and > > > style it via the `:disabled` pseudo class. That way you may be able to > > > completely get rid of `addCustomFilter()`, `removeCustomFilter()` and > > > `convertToRawFormat()`. > > > > Interesting, shouldn't we allow them to delete the filters, otherwise this > will > > be a regression, what you think? Anyway I agree that your suggestion is more > > simple. > > This will not be a regression, as user still can remove the filter from the > textarea. Follow Thomas suggestion regarding implementation. Done. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:136: } On 2017/07/10 13:27:43, saroyanm wrote: > On 2017/07/10 11:38:00, saroyanm wrote: > > On 2017/07/07 13:01:09, Thomas Greiner wrote: > > > What's the reason for those changes? I didn't notice any header being added. > > > > Header is column name. I used naming header from "<TH>" element. > > beforeIndex is increased by one in case table have header column. > > > > While this is specified in the HTML rather than generated dynamically it's > easy > > to use this solution. Anyway let's discuss this implementation. > > I'll move the column names row out of the list for current review. Done. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:229: let timer = setInterval(lastUpdateLive, 60000); On 2017/07/10 11:38:02, saroyanm wrote: > On 2017/07/07 13:01:09, Thomas Greiner wrote: > > As far as I see, the spec doesn't say that this time is supposed to be kept > > up-to-date while the page is open. > I do remember that this was discussed during our meetings and the outcome was to > update this live. Anyway this should end up in the specs as well, if it's not > yet. I'll take care of it. > > > Furthermore, here are some optional improvements in case it turns out that > this > > feature is required: > > - No need to update it every minute if `lastUpdate > 1 hour` > agree > > - The next minute might be reached even before the 60 seconds have passed > since > agree > > it's unlikely that we set the interval at exactly zero seconds. > > - The interval seems to continue even after the item gets removed. > Good point, I'll remove it. Removed live update https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:234: if(sinceUpdate > 86400000) On 2017/07/10 11:38:01, saroyanm wrote: > On 2017/07/07 13:01:10, Thomas Greiner wrote: > > Detail: Usually, we create constants to make it more obvious what all those > > different numbers are supposed to represent. > > > > See > > - https://hg.adblockplus.org/adblockpluscore/file/tip/lib/synchronizer.js#l33 > > - https://hg.adblockplus.org/adblockpluscore/file/tip/lib/downloader.js#l25 > > Agree Done. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:238: document.documentElement.lang, { month: "short" }); On 2017/07/07 13:01:10, Thomas Greiner wrote: > Actually, while reviewing #5278 I noticed that we could simply pass `undefined` > for the locale value to use the current value. If you remember we did try > passing `null` and omitting the first parameter to achieve that but neither one > worked. > > On another note, we could even go as far as to use > > new Intl.DateTimeFormat(undefined, {year: "numeric", month: "short", day: > "2-digit"}).format(new Date()) > > instead to format the date but it doesn't seem like we could get the format "DD > MMM YYYY" out of it so it doesn't seem too useful for what we need here. Done. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:238: document.documentElement.lang, { month: "short" }); On 2017/07/07 13:01:10, Thomas Greiner wrote: > Coding style: This violates the rule "object-curly-spacing" from > eslint-config-eyeo. > > See > - > https://hg.adblockplus.org/codingtools/file/tip/eslint-config-eyeo/index.js#l84 > - http://eslint.org/docs/rules/object-curly-spacing Done. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:425: customFiltersView("read"); On 2017/07/10 11:38:01, saroyanm wrote: > On 2017/07/07 13:01:10, Thomas Greiner wrote: > > Detail: The name is ambiguous because it doesn't say what happens with the > > custom filters view. Names such as "setCustomFiltersView" or > > "switchCustomFiltersView" would clarify that. > > Agree. Done. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:537: if (element.getAttribute("href") == "#") On 2017/07/10 11:38:00, saroyanm wrote: > On 2017/07/07 13:01:10, Thomas Greiner wrote: > > Detail: Why do you even set the "href" attribute in the first place? > > Good question, I think we don't need href attribute, at all, I thought it's some > kind of common practice to specify href attribute, but I think we can remove it. Done. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:663: function customFiltersView(mode) On 2017/07/10 11:38:00, saroyanm wrote: > On 2017/07/07 13:01:09, Thomas Greiner wrote: > > Detail: I noticed that you've call `E("custom-filters")` quite often and > > unconditionally so let's introduce a variable for that. > > Agree. Done. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:663: function customFiltersView(mode) On 2017/07/10 11:38:00, saroyanm wrote: > On 2017/07/07 13:01:09, Thomas Greiner wrote: > > You're trying to switch between three modes but each class can only represent > > two values. The simplest solutions for this scenario would be to either not > use > > classes (e.g. `data-mode="view"`) or to use two classes (e.g. `.empty.view`). > > ".mode-edit" Will be consistent with other implementation we already introduced, > previously. But I agree that data-mode="view" is more clear implementation. I > think that we are safe to switch to using data attributes, that will be > consistent with how we are switching between tabs. And inconsistency between > using class attribute will be removed after #5391 is resolved. Done. https://codereview.adblockplus.org/29445590/diff/29465584/new-options.js#newc... new-options.js:674: E("custom-filters").classList.add("mode-view"); On 2017/07/10 11:38:00, saroyanm wrote: > On 2017/07/07 13:01:10, Thomas Greiner wrote: > > Detail: Why the different naming? I do like that you chose to go with "read" > and > > "write" instead of "view" and "edit" but why not use them consistently > > throughout? > > Acknowledged, will make them consistent Done. https://codereview.adblockplus.org/29445590/diff/29465584/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29445590/diff/29465584/skin/new-options.cs... skin/new-options.css:699: #all-filter-lists-table .static label On 2017/07/10 11:38:02, saroyanm wrote: > On 2017/07/07 13:01:11, Thomas Greiner wrote: > > Detail: You removed the only element with the "static" class so it seems that > we > > can remove this now. > > Well spotted. Done.
Added a few small comments. In general the code looks much simpler now since you were able to remove the `<select>` element so thanks for doing that. https://codereview.adblockplus.org/29445590/diff/29487616/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29445590/diff/29487616/locale/en-US/new-op... locale/en-US/new-options.json:1: { Detail: Looks like you accidentally added this whitespace. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:30: let customFiltersArray = []; Detail: I'd recommend not to include the value type in the variable name. For instance, the variable name "customFiltersListElement" will need to be updated if we decide not to use `<li>` elements to represent those anymore. Something more generic like "customFilters" instead of "customFiltersArray" and "customFiltersElement" instead of "customFiltersListElement" would already be sufficient to avoid such inconsistencies. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:248: getMessage("options_filterList_hours", placeholder); This placeholder isn't defined in new-options.json and in the last meeting we agreed on leaving out the numbers as a compromise to not having to implement the live updating of the dates. Therefore there'd be no need for this placeholder. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:419: if (isCustomFiltersLoaded) Detail: What is this check for? It seems that `updateCustomFiltersUi()` isn't doing anything that requires custom filters to be loaded. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:430: customFiltersArray.splice(index, 1); What if there are multiple instances of the same filter? https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:438: customFiltersListElement.value = ""; Detail: This is redundant because the line below will have the same effect if `customFiltersArray` is empty. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:667: updateCustomFiltersUi(); Interesting that you're calling `updateCustomFiltersUi()` only for the mode "write". Is there a reason why you're not calling it unconditionally to ensure that the list will always be up-to-date whenever switching its mode?
Thanks Thomas I'd like to clarify 2 comments, before uploading new patch. https://codereview.adblockplus.org/29445590/diff/29487616/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29445590/diff/29487616/locale/en-US/new-op... locale/en-US/new-options.json:1: { On 2017/07/14 12:26:12, Thomas Greiner wrote: > Detail: Looks like you accidentally added this whitespace. Right, this is fixed in the "rebase" patch. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:30: let customFiltersArray = []; On 2017/07/14 12:26:13, Thomas Greiner wrote: > Detail: I'd recommend not to include the value type in the variable name. For > instance, the variable name "customFiltersListElement" will need to be updated > if we decide not to use `<li>` elements to represent those anymore. > > Something more generic like "customFilters" instead of "customFiltersArray" and > "customFiltersElement" instead of "customFiltersListElement" would already be > sufficient to avoid such inconsistencies. Acknowledged. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:248: getMessage("options_filterList_hours", placeholder); On 2017/07/14 12:26:12, Thomas Greiner wrote: > This placeholder isn't defined in new-options.json and in the last meeting we > agreed on leaving out the numbers as a compromise to not having to implement the > live updating of the dates. > > Therefore there'd be no need for this placeholder. Well spotted, will remove it. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:419: if (isCustomFiltersLoaded) On 2017/07/14 12:26:12, Thomas Greiner wrote: > Detail: What is this check for? It seems that `updateCustomFiltersUi()` isn't > doing anything that requires custom filters to be loaded. Shouldn't we wait only for the time when all filters are loaded to update the view ? Otherwise the view will be updated for each filter, on start. ex.: When user have a lot of filters, this might have performance impact. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:430: customFiltersArray.splice(index, 1); On 2017/07/14 12:26:12, Thomas Greiner wrote: > What if there are multiple instances of the same filter? There shouldn't be multiple instances of same filter AFAIK. Extension should not allow having duplicated instances ? https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:438: customFiltersListElement.value = ""; On 2017/07/14 12:26:12, Thomas Greiner wrote: > Detail: This is redundant because the line below will have the same effect if > `customFiltersArray` is empty. Thanks for noticing. Will update. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:667: updateCustomFiltersUi(); On 2017/07/14 12:26:13, Thomas Greiner wrote: > Interesting that you're calling `updateCustomFiltersUi()` only for the mode > "write". Is there a reason why you're not calling it unconditionally to ensure > that the list will always be up-to-date whenever switching its mode? You are right, I'm not sure why I did this.. I'll fix this
https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:419: if (isCustomFiltersLoaded) On 2017/07/14 16:17:25, saroyanm wrote: > On 2017/07/14 12:26:12, Thomas Greiner wrote: > > Detail: What is this check for? It seems that `updateCustomFiltersUi()` isn't > > doing anything that requires custom filters to be loaded. > Shouldn't we wait only for the time when all filters are loaded to update the > view ? Otherwise the view will be updated for each filter, on start. ex.: When > user have a lot of filters, this might have performance impact. Right, I remember and I see that this has actually already been there before. Presumably, the most appropriate way would be to handle this within the collection but let's leave it like this for now since it works. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:430: customFiltersArray.splice(index, 1); On 2017/07/14 16:17:25, saroyanm wrote: > On 2017/07/14 12:26:12, Thomas Greiner wrote: > > What if there are multiple instances of the same filter? > > There shouldn't be multiple instances of same filter AFAIK. Extension should not > allow having duplicated instances ? My thinking was that a pre-existing filter could already be duplicated even before our UI has had the chance to check for that. But seeing that the current options page also checks for duplicates and also doesn't consider this circumstance, we should be safe if we simply ignore this.
https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:419: if (isCustomFiltersLoaded) On 2017/07/14 16:37:42, Thomas Greiner wrote: > On 2017/07/14 16:17:25, saroyanm wrote: > > On 2017/07/14 12:26:12, Thomas Greiner wrote: > > > Detail: What is this check for? It seems that `updateCustomFiltersUi()` > isn't > > > doing anything that requires custom filters to be loaded. > > Shouldn't we wait only for the time when all filters are loaded to update the > > view ? Otherwise the view will be updated for each filter, on start. ex.: When > > user have a lot of filters, this might have performance impact. > > Right, I remember and I see that this has actually already been there before. > > Presumably, the most appropriate way would be to handle this within the > collection but let's leave it like this for now since it works. I agree, I'll create a separate issue for that, when this review is resolved. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:430: customFiltersArray.splice(index, 1); On 2017/07/14 16:37:42, Thomas Greiner wrote: > On 2017/07/14 16:17:25, saroyanm wrote: > > On 2017/07/14 12:26:12, Thomas Greiner wrote: > > > What if there are multiple instances of the same filter? > > > > There shouldn't be multiple instances of same filter AFAIK. Extension should > not > > allow having duplicated instances ? > > My thinking was that a pre-existing filter could already be duplicated even > before our UI has had the chance to check for that. But seeing that the current > options page also checks for duplicates and also doesn't consider this > circumstance, we should be safe if we simply ignore this. Acknowledged.
New patch uploaded, ready for the review. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:30: let customFiltersArray = []; On 2017/07/14 16:17:25, saroyanm wrote: > On 2017/07/14 12:26:13, Thomas Greiner wrote: > > Detail: I'd recommend not to include the value type in the variable name. For > > instance, the variable name "customFiltersListElement" will need to be updated > > if we decide not to use `<li>` elements to represent those anymore. > > > > Something more generic like "customFilters" instead of "customFiltersArray" > and > > "customFiltersElement" instead of "customFiltersListElement" would already be > > sufficient to avoid such inconsistencies. > > Acknowledged. Done. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:248: getMessage("options_filterList_hours", placeholder); On 2017/07/14 16:17:24, saroyanm wrote: > On 2017/07/14 12:26:12, Thomas Greiner wrote: > > This placeholder isn't defined in new-options.json and in the last meeting we > > agreed on leaving out the numbers as a compromise to not having to implement > the > > live updating of the dates. > > > > Therefore there'd be no need for this placeholder. > > Well spotted, will remove it. Done. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:438: customFiltersListElement.value = ""; On 2017/07/14 16:17:24, saroyanm wrote: > On 2017/07/14 12:26:12, Thomas Greiner wrote: > > Detail: This is redundant because the line below will have the same effect if > > `customFiltersArray` is empty. > > Thanks for noticing. Will update. Done. https://codereview.adblockplus.org/29445590/diff/29487616/new-options.js#newc... new-options.js:667: updateCustomFiltersUi(); On 2017/07/14 16:17:25, saroyanm wrote: > On 2017/07/14 12:26:13, Thomas Greiner wrote: > > Interesting that you're calling `updateCustomFiltersUi()` only for the mode > > "write". Is there a reason why you're not calling it unconditionally to ensure > > that the list will always be up-to-date whenever switching its mode? > > You are right, I'm not sure why I did this.. I'll fix this Done. https://codereview.adblockplus.org/29445590/diff/29487656/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29487656/new-options.js#newc... new-options.js:1061: function editCustomFilters() This is not used anymore.
There's one unused argument that I found and one suggestion but apart from that: LGTM. https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29445590/diff/29465584/locale/en-US/new-op... locale/en-US/new-options.json:172: "message": "CUSTOMIZE ON-PAGE ACTIONS" On 2017/07/10 11:37:59, saroyanm wrote: > On 2017/07/07 13:01:08, Thomas Greiner wrote: > > Detail: Don't capitalize the text here or otherwise we'd have to redo the > > translation whenever we decide to capitalize the text differently. Instead use > > `text-transform: uppercase;` in the CSS. > > > > Same applies to "options_customFilters_title". > > My idea was to capitalize and rollback to the lower case after introduction of > CSS UPPER_CASE, but while you mentioned this, I'll follow your suggestion. Ok, as long as we don't forget to add the necessary styles for transforming the text to upper-case later on it's fine with me. https://codereview.adblockplus.org/29445590/diff/29487616/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29445590/diff/29487616/locale/en-US/new-op... locale/en-US/new-options.json:1: { On 2017/07/14 16:17:24, saroyanm wrote: > On 2017/07/14 12:26:12, Thomas Greiner wrote: > > Detail: Looks like you accidentally added this whitespace. > > Right, this is fixed in the "rebase" patch. Acknowledged. https://codereview.adblockplus.org/29445590/diff/29489581/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29445590/diff/29489581/locale/en-US/new-op... locale/en-US/new-options.json:182: "message": "customize on-page actions" Suggestion: Even though we're transforming this text into upper-case via CSS, it should still look consistent if we decide to drop that style later on. So since all other texts - except for actions - start with an upper-case letter, I'd recommend to have the same logic here. Same applies to "options_customFilters_title". https://codereview.adblockplus.org/29445590/diff/29489581/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29489581/new-options.js#newc... new-options.js:543: function execAction(action, element, event) Detail: `event` is not being used anymore so please remove it from here and from all instances where this function is called.
Fixed, thanks! https://codereview.adblockplus.org/29445590/diff/29489581/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29445590/diff/29489581/locale/en-US/new-op... locale/en-US/new-options.json:182: "message": "customize on-page actions" On 2017/07/14 17:36:12, Thomas Greiner wrote: > Suggestion: Even though we're transforming this text into upper-case via CSS, it > should still look consistent if we decide to drop that style later on. So since > all other texts - except for actions - start with an upper-case letter, I'd > recommend to have the same logic here. > > Same applies to "options_customFilters_title". Done. https://codereview.adblockplus.org/29445590/diff/29489581/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29445590/diff/29489581/new-options.js#newc... new-options.js:543: function execAction(action, element, event) On 2017/07/14 17:36:12, Thomas Greiner wrote: > Detail: `event` is not being used anymore so please remove it from here and from > all instances where this function is called. Done.
On 2017/07/14 17:45:10, saroyanm wrote: > Fixed, thanks! > > https://codereview.adblockplus.org/29445590/diff/29489581/locale/en-US/new-op... > File locale/en-US/new-options.json (right): > > https://codereview.adblockplus.org/29445590/diff/29489581/locale/en-US/new-op... > locale/en-US/new-options.json:182: "message": "customize on-page actions" > On 2017/07/14 17:36:12, Thomas Greiner wrote: > > Suggestion: Even though we're transforming this text into upper-case via CSS, > it > > should still look consistent if we decide to drop that style later on. So > since > > all other texts - except for actions - start with an upper-case letter, I'd > > recommend to have the same logic here. > > > > Same applies to "options_customFilters_title". > > Done. > > https://codereview.adblockplus.org/29445590/diff/29489581/new-options.js > File new-options.js (right): > > https://codereview.adblockplus.org/29445590/diff/29489581/new-options.js#newc... > new-options.js:543: function execAction(action, element, event) > On 2017/07/14 17:36:12, Thomas Greiner wrote: > > Detail: `event` is not being used anymore so please remove it from here and > from > > all instances where this function is called. > > Done. LGTM again. :) 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
