options.html - Issue 29321198: Issue 2376 - Implement custom filters in new options page

Delta Between Two Patch Sets: options.html

Issue 29321198: Issue 2376 - Implement custom filters in new options page (Closed)

Left Patch Set: Created June 29, 2015, 11:21 a.m.

Right Patch Set: Nit fixes Created July 15, 2015, 2:35 p.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-2015 Eyeo GmbH	4 - Copyright (C) 2006-2015 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 19 matching lines...) Expand all Loading...
30 <body data-tab="general">	30 <body data-tab="general">

31 <!-- Navigation sidebar -->	31 <!-- Navigation sidebar -->

32 <div id="nav-sidebar">	32 <div id="nav-sidebar">

33 <div id="fixed-sidebar" class="fixed">	33 <div id="fixed-sidebar" class="fixed">

34 <div id="page-title">	34 <div id="page-title">

35 <p class="i18n_options_page_header_1"></p>	35 <p class="i18n_options_page_header_1"></p>

36 <h1 class="i18n_options_page_header_2"></h1>	36 <h1 class="i18n_options_page_header_2"></h1>

37 </div>	37 </div>

38 <ul id="main-navigation-tabs" class="tabs vertical">	38 <ul id="main-navigation-tabs" class="tabs vertical">

39 <li id="tab-general" data-show="general" class="active">	39 <li id="tab-general" data-show="general" class="active">

40 <span class="i18n_options_tab_general"></span>	40 <a class="i18n_options_tab_general"></a>

41 <span class="icon"></span>	41 <span class="icon"></span>

42 </li>	42 </li>

43 <li id="tab-advanced" data-show="advanced">	43 <li id="tab-advanced" data-show="advanced">

44 <span class="i18n_options_tab_advanced"></span>	44 <a class="i18n_options_tab_advanced"></a>

45 <span class="icon"></span>	45 <span class="icon"></span>

46 </li>	46 </li>

47 <li id="tab-help" data-show="help">	47 <li id="tab-help" data-show="help">

48 <span class="i18n_options_tab_help"></span>	48 <a class="i18n_options_tab_help"></a>

49 <span class="icon"></span>	49 <span class="icon"></span>

50 </li>	50 </li>

51 </ul>	51 </ul>

52 <p class="nav-link">	52 <a id="link-version">

53 <span class="i18n_options_version"></span> <span id="abp-version"></sp an>	53 <span class="i18n_options_version"></span>

54 </p>	54 <span id="abp-version"></span>

	55 </a>

55 <ul class="tabs vertical bottom">	56 <ul class="tabs vertical bottom">

56 <li id="tab-share">	57 <li id="tab-share">

57 <span class="i18n_options_tab_share"></span><span class="icon"></spa n>	58 <a class="i18n_options_tab_share"></a>

58 </li>	59 <span class="icon"></span>

59 <li id="tab-donate">	60 </li>

60 <span class="i18n_options_tab_donate"></span><span class="icon"></sp an>	61 <li id="tab-contribute">

	62 <a class="i18n_options_tab_contribute"></a>

	63 <span class="icon"></span>

61 </li>	64 </li>

62 </ul>	65 </ul>

63 </div>	66 </div>

64 </div>	67 </div>

65 <div id="tab-content">	68 <div id="tab-content">

66 <div id="content-wrapper">	69 <div id="content-wrapper">

67 <div id="dialog-background"></div>	70 <div id="dialog-background"></div>

68	71

69 <!-- General tab content -->	72 <!-- General tab content -->

70 <div id="content-general">	73 <div id="content-general">

(...skipping 88 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
159 </div>	162 </div>

160 </div>	163 </div>

161 </div>	164 </div>

162 </div>	165 </div>

163 </div>	166 </div>

164 </div>	167 </div>

165	168

166 <!-- Advanced tab content -->	169 <!-- Advanced tab content -->

167 <div id="content-advanced">	170 <div id="content-advanced">

168 <div>	171 <div>

169 <h1><span class="i18n_options_tweaks_title"></span><a class="i18n_op tions_readMore tooltip" href="#"></a></h1>	172 <h1><span class="i18n_options_tweaks_title"></span><a class="i18n_op tions_readMore tooltip" href="#"></a></h1>
Thomas Greiner 2015/06/30 09:23:27 While I did intentionally ignore the Advanced tab While I did intentionally ignore the Advanced tab in the review for #1524, I still expect the same issues to be fixed in this one if they occur. So please don't squish everything into a single line. This is not legible and difficult to maintain. saroyanm 2015/07/08 18:25:40 I didn't touch this part of advanced tab intention Show quoted text On 2015/06/30 09:23:27, Thomas Greiner wrote: > While I did intentionally ignore the Advanced tab in the review for #1524, I > still expect the same issues to be fixed in this one if they occur. > > So please don't squish everything into a single line. This is not legible and > difficult to maintain. I didn't touch this part of advanced tab intentionally, I was only working on the CustomFilters block, were expecting to start with this part with #2377 and with blocking list part with #2375. Not sure if this fixes goes to this ticket. Thomas Greiner 2015/07/09 11:07:54 Fair enough. As long as it's being tackled it shou Show quoted text On 2015/07/08 18:25:40, saroyanm wrote: > On 2015/06/30 09:23:27, Thomas Greiner wrote: > > While I did intentionally ignore the Advanced tab in the review for #1524, I > > still expect the same issues to be fixed in this one if they occur. > > > > So please don't squish everything into a single line. This is not legible and > > difficult to maintain. > > I didn't touch this part of advanced tab intentionally, I was only working on > the CustomFilters block, were expecting to start with this part with #2377 and > with blocking list part with #2375. Not sure if this fixes goes to this ticket. Fair enough. As long as it's being tackled it should be fine.
170 <ul class="table" style="width: auto;">	173 <ul class="table" style="width: auto;">

171 <li>	174 <li>

172 <input type="checkbox" id="easylist"/><span id="block-element-ex planation" class="i18n_options_tweaks_blockElement"></span></li>	175 <input type="checkbox" id="easylist"/><span id="block-element-ex planation" class="i18n_options_tweaks_blockElement"></span></li>

173 </ul>	176 </ul>

174 </div>	177 </div>

175 <div>	178 <div>

176 <h1><span class="i18n_options_blockingList_title"></span><a class="i 18n_options_readMore tooltip" href="#" target="_blank"></a></h1>	179 <h1><span class="i18n_options_blockingList_title"></span><a class="i 18n_options_readMore tooltip" href="#" target="_blank"></a></h1>

177 <ul id="blocking-list-tabs" class="tabs horizontal">	180 <ul id="blocking-list-tabs" class="tabs horizontal">

178 <li class="i18n_options_tab_overview active" data-show="blocking-l ist-overview"></li><li class="i18n_options_tab_ownList" data-show="custom-filter s"></li>	181 <li class="i18n_options_tab_overview active" data-show="blocking-l ist-overview"></li><li class="i18n_options_tab_ownList" data-show="custom-filter s"></li>

179 </ul>	182 </ul>

180 <div id="blocking-list">	183 <div id="blocking-list">

181 <div id="blocking-list-overview">	184 <div id="blocking-list-overview">

182 <ul class="table cols" style="width: auto;">	185 <ul class="table cols" style="width: auto;">

183 <li class="col-name"><span class="i18n_options_tableCol_name"> </span><span class="i18n_options_tableCol_description"></span><span class="i18n_ options_tableCol_date"></span></li>	186 <li class="col-name"><span class="i18n_options_tableCol_name"> </span><span class="i18n_options_tableCol_description"></span><span class="i18n_ options_tableCol_date"></span></li>

184 <li><input type="checkbox" id="easylist"/><span>Easylist</span ><span>Adblocking english sites</span><span>15 March 14 - 10:31</span></li>	187 <li><input type="checkbox" id="easylist"/><span>Easylist</span ><span>Adblocking english sites</span><span>15 March 14 - 10:31</span></li>

185 <li><input type="checkbox" id="easylist+de"/><span>Easylist Ge rmany + Easylist</span><span>Adblocking english + german sites</span><span>15 Ma rch 14 - 10:31</span></li>	188 <li><input type="checkbox" id="easylist+de"/><span>Easylist Ge rmany + Easylist</span><span>Adblocking english + german sites</span><span>15 Ma rch 14 - 10:31</span></li>

186 <li><input type="checkbox" id="annoyance-fb"/><span>Facebook a nnoyance blocker</span><span>Blocks Facebook annoyances</span><span>15 March 14 - 10:31</span></li>	189 <li><input type="checkbox" id="annoyance-fb"/><span>Facebook a nnoyance blocker</span><span>Blocks Facebook annoyances</span><span>15 March 14 - 10:31</span></li>

187 <li><input type="checkbox" id="annoyance-youtube"/><span>Faceb ook annoyance blocker</span><span>Blocks Facebook annoyances</span><span>15 Marc h 14 - 10:31</span></li>	190 <li><input type="checkbox" id="annoyance-youtube"/><span>Faceb ook annoyance blocker</span><span>Blocks Facebook annoyances</span><span>15 Marc h 14 - 10:31</span></li>

188 <li><input type="checkbox" id="own-list"/><span>Own blocking l ist</span><span>Your own blocking list</span><span><a href="#">edit your blockin g list</a></span></li>	191 <li><input type="checkbox" id="own-list"/><span>Own blocking l ist</span><span>Your own blocking list</span><span><a href="#">edit your blockin g list</a></span></li>

189 </ul>	192 </ul>

190 <div class="controls">	193 <div class="controls">

191 <button>	194 <button>

192 <span class="icon icon-add"></span><span class="i18n_options _blockingList_add"></span>	195 <span class="icon icon-add"></span><span class="i18n_options _blockingList_add"></span>

193 </button>	196 </button>

194 <button>	197 <button>

195 <span class="icon icon-update"></span><span class="i18n_opti ons_blockingList_update"></span>	198 <span class="icon icon-update"></span><span class="i18n_opti ons_blockingList_update"></span>

196 </button>	199 </button>

197 </div>	200 </div>

198 </div>	201 </div>

199 <div id="custom-filters" data-view="list">	202 <div id="custom-filters">

200 <h4 id="custom-filters-header" class="i18n_options_customFilters _title"></h4>	203 <h2 id="custom-filters-header" class="i18n_options_customFilters _title"></h2>
Thomas Greiner 2015/06/30 09:23:27 Where did <h2> and <h3> go? Where did <h2> and <h3> go? saroyanm 2015/07/08 18:25:40 You are right we don't have h4 in mock up, h2 shou Show quoted text On 2015/06/30 09:23:27, Thomas Greiner wrote: > Where did <h2> and <h3> go? You are right we don't have h4 in mock up, h2 should go here.
201 <div id="custom-filters-list-wrapper">	204 <div id="custom-filters-wrapper">

202 <ul id="custom-filters-table" class="table list">	205 <div id="custom-filters-list-wrapper">

203 <template>	206 <ul id="custom-filters-table" class="table list">

204 <span class="display"></span>	207 <template>

205 </template>	208 <span class="display"></span>

206 </ul>	209 </template>

207 <div id="custom-filter-add-wrapper" class="controls">	210 </ul>
Thomas Greiner 2015/06/30 09:23:27 Tip: You could avoid that "Enter" key check by usi Tip: You could avoid that "Enter" key check by using <form> and simply listening to its "submit" event. Thomas Greiner 2015/06/30 09:23:27 I noticed that almost every single element has an I noticed that almost every single element has an ID. For instance, for this one I couldn't even find anywhere in the JavaScript or CSS code where you reference it via its ID. Therefore please make sure for all elements that there's no other way to uniquely identify them other than an ID (e.g. #custom-filters-add-btn could be referenced via `#custom-filters-add button` in which case you can shorten the ID of #custom-filters-add-wrapper to #custom-filters-add). saroyanm 2015/07/08 18:25:40 Good point, make sense to also handle in consisten Show quoted text On 2015/06/30 09:23:27, Thomas Greiner wrote: > Tip: You could avoid that "Enter" key check by using <form> and simply listening > to its "submit" event. Good point, make sense to also handle in consistency related ticket the whitelisting part. Added to the document. saroyanm 2015/07/08 18:25:40 Referencing here -> https://codereview.adblockplus Show quoted text On 2015/06/30 09:23:27, Thomas Greiner wrote: > I noticed that almost every single element has an ID. For instance, for this one > I couldn't even find anywhere in the JavaScript or CSS code where you reference > it via its ID. Referencing here -> https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne... Show quoted text > Therefore please make sure for all elements that there's no other way to > uniquely identify them other than an ID (e.g. #custom-filters-add-btn could be > referenced via `#custom-filters-add button` in which case you can shorten the ID > of #custom-filters-add-wrapper to #custom-filters-add). Fare enough done.
208 <input id="custom-filters-add-textbox" type="text"/>	211 <form id="custom-filters-add" class="controls">

209 <button id="custom-filters-add-btn" class="i18n_options_butt on_add"></button>	212 <input type="text"/>

	213 <button class="i18n_options_button_add" type="submit"></bu tton>

	214 </form>

210 </div>	215 </div>

211 </div>	216 <textarea id="custom-filters-raw"></textarea>

212 <textarea id="custom-filters-textarea"></textarea>	217 </div>
Thomas Greiner 2015/06/30 09:23:27 As mentioned in the previous review, IDs or class As mentioned in the previous review, IDs or class names should not refer to a specific UI element because that can change at some point. Therefore it's more flexible if you use something more generic like "custom-filters-raw" or "custom-filters-modify". saroyanm 2015/07/08 18:25:40 Done. Show quoted text On 2015/06/30 09:23:27, Thomas Greiner wrote: > As mentioned in the previous review, IDs or class names should not refer to a > specific UI element because that can change at some point. Therefore it's more > flexible if you use something more generic like "custom-filters-raw" or > "custom-filters-modify". Done.
213 <div id="custom-filters-edit-wrapper" class="controls">	218 <div id="custom-filters-edit-wrapper" class="controls">

214 <button id="custom-filters-edit-btn" data-show="edit">	219 <button id="custom-filters-show-edit">

215 <span class="icon icon-edit"></span>	220 <span class="icon icon-edit"></span>

216 <span class="i18n_options_customFilter_edit_btn"></span>	221 <span class="i18n_options_customFilter_edit"></span>

217 </button>	222 </button>

218 <div id="custom-filters-textarea-controls">	223 <div id="custom-filters-raw-controls">

219 <button id="custom-filters-list-btn" data-show="list">	224 <button>

220 <span class="icon icon-edit"></span>	225 <span class="icon icon-edit"></span>

221 <span class="i18n_options_customFilter_list_btn"></span>	226 <span class="i18n_options_customFilter_list"></span>

222 </button>	227 </button>

223 <button id="custom-filters-save-btn" data-show="list">	228 <button id="custom-filters-raw-save">

224 <span class="icon icon-edit"></span>	229 <span class="icon icon-edit"></span>

225 <span class="i18n_options_customFilter_save_btn"></span>	230 <span class="i18n_options_customFilter_save"></span>

226 </button>	231 </button>

227 </div>	232 </div>

228 </div>	233 </div>

229 </div>	234 </div>

230 </div>	235 </div>

231 </div>	236 </div>

232 </div>	237 </div>

233	238

234 <!-- Help tab content -->	239 <!-- Help tab content -->

235 <div id="content-help">	240 <div id="content-help">

(...skipping 69 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
305 <h3 class="i18n_options_dialog_edit_own_list"></h3>	310 <h3 class="i18n_options_dialog_edit_own_list"></h3>

306 <div id="edit-ownBlockingList-button" class="button-wrapper">	311 <div id="edit-ownBlockingList-button" class="button-wrapper">

307 <span class="icon icon-arrow"></span>	312 <span class="icon icon-arrow"></span>

308 <span class="i18n_options_dialog_create_own_list"></span>	313 <span class="i18n_options_dialog_create_own_list"></span>

309 </div>	314 </div>

310 </div>	315 </div>

311 </div>	316 </div>

312 </div>	317 </div>

313 </div>	318 </div>

314 </body>	319 </body>

315 </html>	320 </html>

LEFT	RIGHT