options.html - Issue 29337729: Issue 2374 - Implemented Tweaks section in options page

Delta Between Two Patch Sets: options.html

Issue 29337729: Issue 2374 - Implemented Tweaks section in options page (Closed)

Left Patch Set: Created Feb. 25, 2016, 5:50 p.m.

Right Patch Set: Created March 16, 2016, 11:13 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <!--	2 <!--

3 - This file is part of Adblock Plus <https://adblockplus.org/>,	3 - This file is part of Adblock Plus <https://adblockplus.org/>,

4 - Copyright (C) 2006-2016 Eyeo GmbH	4 - Copyright (C) 2006-2016 Eyeo GmbH

5 -	5 -

6 - Adblock Plus is free software: you can redistribute it and/or modify	6 - Adblock Plus is free software: you can redistribute it and/or modify

7 - it under the terms of the GNU General Public License version 3 as	7 - it under the terms of the GNU General Public License version 3 as

8 - published by the Free Software Foundation.	8 - published by the Free Software Foundation.

9 -	9 -

10 - Adblock Plus is distributed in the hope that it will be useful,	10 - Adblock Plus is distributed in the hope that it will be useful,

(...skipping 194 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
205 <button class="i18n_options_button_cancel cancel-button" dat a-action="cancel-domain-exception"></button>	205 <button class="i18n_options_button_cancel cancel-button" dat a-action="cancel-domain-exception"></button>

206 </div>	206 </div>

207 </div>	207 </div>

208 </div>	208 </div>

209 </div>	209 </div>

210 </div>	210 </div>

211 </div>	211 </div>

212	212

213 <!-- Advanced tab content -->	213 <!-- Advanced tab content -->

214 <div id="content-advanced" class="tab-content">	214 <div id="content-advanced" class="tab-content">

215 <div id="tweaks">	215 <div>

216 <h1>	216 <h1>

217 <span class="i18n_options_tweaks_title"></span>	217 <span class="i18n_options_tweaks_title"></span>

218 <a class="i18n_options_readMore tooltip"></a>	218 <a class="i18n_options_readMore tooltip"></a>

219 </h1>	219 </h1>

220 <ul class="table">	220 <ul id="tweaks" class="table">

221 <li>	221 <li data-pref="shouldShowBlockElementMenu">

222 <button role="checkbox" name="shouldShowBlockElementMenu" data-a ction="toggle-pref"></button>	222 <button role="checkbox" data-action="toggle-pref"></button>
saroyanm 2016/02/29 14:34:39 Why we are using name attribute here ? I think we Why we are using name attribute here ? I think we can do this consistent with other similar elements, use access attribute on parent element. Thomas Greiner 2016/02/29 17:32:43 Done. I thought going with existing attributes whe Show quoted text On 2016/02/29 14:34:39, saroyanm wrote: > Why we are using name attribute here ? I think we can do this consistent with > other similar elements, use access attribute on parent element. Done. I thought going with existing attributes where possible might be the better choice but don't have a strong preference so I changed it to `data-pref`. saroyanm 2016/03/11 14:55:58 Not exactly what I meant, I meant something like: Show quoted text On 2016/02/29 17:32:43, Thomas Greiner wrote: > On 2016/02/29 14:34:39, saroyanm wrote: > > Why we are using name attribute here ? I think we can do this consistent with > > other similar elements, use access attribute on parent element. > > Done. I thought going with existing attributes where possible might be the > better choice but don't have a strong preference so I changed it to `data-pref`. Not exactly what I meant, I meant something like: <li data-access="shouldShowBlockElementMenu"> <button role="checkbox" data-action="toggle-pref"></button> <span class="i18n_options_tweaks_blockElement"></span> </li> and use: ext.backgroundPage.sendMessage( { type: "prefs.toggle", key: findParentData(element, "access", false) }); Also you can have an ID on UL elements in tweaks section ex.: tweaks-prefs. And that will make the tables consistent IMO and access to them as well: var tweaks = document.querySelectorAll("#tweaks-prefs li"); Anyway this requires to always access parent element, which in one hand additional action, but on other hand provide ability to reference all related elements, regardless it's related string or checkbox. Thomas Greiner 2016/03/15 15:39:25 Done. Show quoted text On 2016/03/11 14:55:58, saroyanm wrote: > On 2016/02/29 17:32:43, Thomas Greiner wrote: > > On 2016/02/29 14:34:39, saroyanm wrote: > > > Why we are using name attribute here ? I think we can do this consistent > with > > > other similar elements, use access attribute on parent element. > > > > Done. I thought going with existing attributes where possible might be the > > better choice but don't have a strong preference so I changed it to > `data-pref`. > > Not exactly what I meant, I meant something like: > <li data-access="shouldShowBlockElementMenu"> > <button role="checkbox" data-action="toggle-pref"></button> > <span class="i18n_options_tweaks_blockElement"></span> > </li> > > and use: > ext.backgroundPage.sendMessage( > { > type: "prefs.toggle", > key: findParentData(element, "access", false) > }); > > Also you can have an ID on UL elements in tweaks section ex.: tweaks-prefs. And > that will make the tables consistent IMO and access to them as well: > var tweaks = document.querySelectorAll("#tweaks-prefs li"); > > Anyway this requires to always access parent element, which in one hand > additional action, but on other hand provide ability to reference all related > elements, regardless it's related string or checkbox. Done. saroyanm 2016/03/16 10:59:18 Mostly looks good, just small Detail from the comm Show quoted text On 2016/03/15 15:39:25, Thomas Greiner wrote: > On 2016/03/11 14:55:58, saroyanm wrote: > > On 2016/02/29 17:32:43, Thomas Greiner wrote: > > > On 2016/02/29 14:34:39, saroyanm wrote: > > > > Why we are using name attribute here ? I think we can do this consistent > > with > > > > other similar elements, use access attribute on parent element. > > > > > > Done. I thought going with existing attributes where possible might be the > > > better choice but don't have a strong preference so I changed it to > > `data-pref`. > > > > Not exactly what I meant, I meant something like: > > <li data-access="shouldShowBlockElementMenu"> > > <button role="checkbox" data-action="toggle-pref"></button> > > <span class="i18n_options_tweaks_blockElement"></span> > > </li> > > > > and use: > > ext.backgroundPage.sendMessage( > > { > > type: "prefs.toggle", > > key: findParentData(element, "access", false) > > }); > > > > Also you can have an ID on UL elements in tweaks section ex.: tweaks-prefs. > And > > that will make the tables consistent IMO and access to them as well: > > var tweaks = document.querySelectorAll("#tweaks-prefs li"); > > > > Anyway this requires to always access parent element, which in one hand > > additional action, but on other hand provide ability to reference all related > > elements, regardless it's related string or checkbox. > > Done. Mostly looks good, just small Detail from the comment: What about having ID specified on the "table" and access the prefs through the table as mentioned above ? Something like: <ul id="tweaks" class="table"> var tweaks = document.querySelectorAll("#tweak li"); Thomas Greiner 2016/03/16 11:14:36 Done. Show quoted text On 2016/03/16 10:59:18, saroyanm wrote: > On 2016/03/15 15:39:25, Thomas Greiner wrote: > > On 2016/03/11 14:55:58, saroyanm wrote: > > > On 2016/02/29 17:32:43, Thomas Greiner wrote: > > > > On 2016/02/29 14:34:39, saroyanm wrote: > > > > > Why we are using name attribute here ? I think we can do this consistent > > > with > > > > > other similar elements, use access attribute on parent element. > > > > > > > > Done. I thought going with existing attributes where possible might be the > > > > better choice but don't have a strong preference so I changed it to > > > `data-pref`. > > > > > > Not exactly what I meant, I meant something like: > > > <li data-access="shouldShowBlockElementMenu"> > > > <button role="checkbox" data-action="toggle-pref"></button> > > > <span class="i18n_options_tweaks_blockElement"></span> > > > </li> > > > > > > and use: > > > ext.backgroundPage.sendMessage( > > > { > > > type: "prefs.toggle", > > > key: findParentData(element, "access", false) > > > }); > > > > > > Also you can have an ID on UL elements in tweaks section ex.: tweaks-prefs. > > And > > > that will make the tables consistent IMO and access to them as well: > > > var tweaks = document.querySelectorAll("#tweaks-prefs li"); > > > > > > Anyway this requires to always access parent element, which in one hand > > > additional action, but on other hand provide ability to reference all > related > > > elements, regardless it's related string or checkbox. > > > > Done. > > Mostly looks good, just small Detail from the comment: What about having ID > specified on the "table" and access the prefs through the table as mentioned > above ? > Something like: > > <ul id="tweaks" class="table"> > > var tweaks = document.querySelectorAll("#tweak li"); Done.
223 <span class="i18n_options_tweaks_blockElement"></span>	223 <span class="i18n_options_tweaks_blockElement"></span>

224 </li>	224 </li>

225 <li data-optional>	225 <li data-pref="show_devtools_panel">

226 <button role="checkbox" name="notifications_ignoredcategories" d ata-action="toggle-pref"></button>	226 <button role="checkbox" data-action="toggle-pref"></button>

	227 <span class="i18n_options_tweaks_showDevToolsPanel"></span>

	228 </li>

	229 <li data-pref="notifications_ignoredcategories">

	230 <button role="checkbox" data-action="toggle-pref"></button>

227 <span class="i18n_options_tweaks_showNotifications"></span>	231 <span class="i18n_options_tweaks_showNotifications"></span>

228 </li>	232 </li>

229 <li data-optional>	233 <li data-pref="safari_contentblocker">

230 <button role="checkbox" name="safari_contentblocker" data-action ="toggle-pref"></button>	234 <button role="checkbox" data-action="toggle-pref"></button>

231 <span class="i18n_options_tweaks_safariContentBlocker"></span>	235 <span class="i18n_options_tweaks_safariContentBlocker"></span>

232 </li>	236 </li>

233 </ul>	237 </ul>

234 </div>	238 </div>

235	239
saroyanm 2016/02/29 14:34:39 Detail: no new row needed here. Detail: no new row needed here. Thomas Greiner 2016/02/29 17:32:43 Why not? It visually separates the Tweaks and the Show quoted text On 2016/02/29 14:34:39, saroyanm wrote: > Detail: no new row needed here. Why not? It visually separates the Tweaks and the Filter Lists sections a bit more than before. saroyanm 2016/03/11 14:55:58 Looks like we currently only separate the main tab Show quoted text On 2016/02/29 17:32:43, Thomas Greiner wrote: > On 2016/02/29 14:34:39, saroyanm wrote: > > Detail: no new row needed here. > > Why not? It visually separates the Tweaks and the Filter Lists sections a bit > more than before. Looks like we currently only separate the main tab content and dialog, but fine with me, we should keep in mind for future to separate all sections in that case I assume. But yes not a major thing to argue.
236 <div>	240 <div>

237 <h1>	241 <h1>

238 <span class="i18n_options_filterList_title"></span>	242 <span class="i18n_options_filterList_title"></span>

239 <a class="i18n_options_readMore tooltip"></a>	243 <a class="i18n_options_readMore tooltip"></a>

240 </h1>	244 </h1>

241 <ul class="tabs horizontal">	245 <ul class="tabs horizontal">

242 <li class="i18n_options_tab_overview active" data-action="switch-t ab" data-tab="advanced-allFilterLists"></li>	246 <li class="i18n_options_tab_overview active" data-action="switch-t ab" data-tab="advanced-allFilterLists"></li>

243 <li class="i18n_options_tab_ownList" data-action="switch-tab" data -tab="advanced-customFilters"></li>	247 <li class="i18n_options_tab_ownList" data-action="switch-tab" data -tab="advanced-customFilters"></li>

244 </ul>	248 </ul>

245 <div id="filter-lists">	249 <div id="filter-lists">

(...skipping 160 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
406 <div class="url"></div>	410 <div class="url"></div>

407 <button class="i18n_options_dialog_predefined_confirm default-focus" data-action="add-predefined-subscription"></button>	411 <button class="i18n_options_dialog_predefined_confirm default-focus" data-action="add-predefined-subscription"></button>

408 </div>	412 </div>

409 </div>	413 </div>

410 </div>	414 </div>

411 <!-- Placeholder element to determine when to wrap focus around -->	415 <!-- Placeholder element to determine when to wrap focus around -->

412 <span class="focus-last" tabindex="0"></span>	416 <span class="focus-last" tabindex="0"></span>

413 </div>	417 </div>

414 </body>	418 </body>

415 </html>	419 </html>

LEFT	RIGHT