desktop-options.html - Issue 29674584: Issue 5549 - Implement missing error handlings for custom filter lists

Side by Side Diff: desktop-options.html

Issue 29674584: Issue 5549 - Implement missing error handlings for custom filter lists (Closed)

Patch Set: put loadCustomFilters back Created Feb. 2, 2018, 2:36 p.m.

Left:
Right:

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

Jump to:

View unified diff | Download patch

OLD	NEW
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-present eyeo GmbH	4 - Copyright (C) 2006-present 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 271 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
282 <div class="side-controls wrap">	282 <div class="side-controls wrap">

283 <button class="i18n_options_filterList_add primary" data-action="ope n-dialog" data-dialog="import"></button>	283 <button class="i18n_options_filterList_add primary" data-action="ope n-dialog" data-dialog="import"></button>

284 <button id="update" class="i18n_options_filterList_update secondary" data-action="update-all-subscriptions"></button>	284 <button id="update" class="i18n_options_filterList_update secondary" data-action="update-all-subscriptions"></button>

285 </div>	285 </div>

286 </section>	286 </section>

287	287

288 <section class="cols">	288 <section class="cols">

289 <header>	289 <header>

290 <h2 class="i18n_options_customFilters_title"></h2>	290 <h2 class="i18n_options_customFilters_title"></h2>

291 <p class="i18n_options_customFilters_description"></p>	291 <p class="i18n_options_customFilters_description"></p>

	292 <p>

	293 <a class="i18n_options_customFilters_learn" id="link-filters-1" ta rget="_blank"></a>

	294 </p>

292 </header>	295 </header>

293 <div id="custom-filters">	296 <div id="custom-filters">

294 <h3 class="i18n_options_customFilters_widget_title"></h3>	297 <h3 class="i18n_options_customFilters_widget_title"></h3>

295 <div id="empty-custom-filters">	298 <div id="empty-custom-filters">

296 <p class="i18n_options_customFilters_tip"></p>	299 <p class="i18n_options_customFilters_tip"></p>

297 <button class="i18n_options_customFilters_start primary" data-acti on="edit-custom-filters">	300 <button class="i18n_options_customFilters_start primary" data-acti on="edit-custom-filters">

298 </button>	301 </button>

299 <p>	302 <p>

300 <a class="i18n_options_customFilters_learn" id="link-filters" ta rget="_blank"></a>	303 <a class="i18n_options_customFilters_learn" id="link-filters-2" target="_blank"></a>
	saroyanm 2018/02/02 16:57:19 Suggestion: What about specifying a class or data- Suggestion: What about specifying a class or data-attribute, in order not to manage ID number orders ? a.giammarchi 2018/02/02 17:22:00 only reason I've used ID is that we use this E(... Show quoted text On 2018/02/02 16:57:19, saroyanm wrote: > Suggestion: What about specifying a class or data-attribute, in order not to > manage ID number orders ? only reason I've used ID is that we use this E(...) function quite a lot to setup languages so I've thought it was somehow a best practice to use IDs (performance, aria-labelleedby, etc) so, is ist OK if I just use a class instead of the following? E("link-filters-1").setAttribute("href", link); E("link-filters-2").setAttribute("href", link); saroyanm 2018/02/02 17:58:57 We are using E only for not writing document.getEl Show quoted text On 2018/02/02 17:22:00, a.giammarchi wrote: > On 2018/02/02 16:57:19, saroyanm wrote: > > Suggestion: What about specifying a class or data-attribute, in order not to > > manage ID number orders ? > > only reason I've used ID is that we use this E(...) function quite a lot to > setup languages so I've thought it was somehow a best practice to use IDs > (performance, aria-labelleedby, etc) > > so, is ist OK if I just use a class instead of the following? > E("link-filters-1").setAttribute("href", link); > E("link-filters-2").setAttribute("href", link); We are using E only for not writing document.getElementById or "document.queryselector("ID")", so feel free to use whatever you think makes most sense here. We also planing to simplify documentation link with issue #6114.
301 </p>	304 </p>

302 </div>	305 </div>

303 <textarea id="custom-filters-raw" rows="15"></textarea>	306 <textarea id="custom-filters-raw" rows="15"></textarea>

304 <div class="side-controls">	307 <div id="custom-filters-edit-area">

305 <button id="custom-filters-edit" class="i18n_options_customFilter_ edit secondary" data-action="edit-custom-filters">	308 <div id="custom-filters-edit-error" class="i18n_options_customFilt ers_edit_error"></div>
	saroyanm 2018/02/02 16:57:19 Nit: You call this element "custom-filters-edit-er Nit: You call this element "custom-filters-edit-error" and the list item "custom-filters-error". I don't understand why the one called "edit" when the other one is not, even oth only appear in edit mode. So I'll suggest to rename this to something more descriptive. ex.: custom-filters-error-message a.giammarchi 2018/02/02 17:22:00 I think this is very subjective. This is an error Show quoted text On 2018/02/02 16:57:19, saroyanm wrote: > Nit: You call this element "custom-filters-edit-error" and the list item > "custom-filters-error". > > I don't understand why the one called "edit" when the other one is not, even oth > only appear in edit mode. > > So I'll suggest to rename this to something more descriptive. ex.: > custom-filters-error-message I think this is very subjective. This is an error about editing within the edit area so I prefer current id. saroyanm 2018/02/02 17:58:57 I see. In general edit-area IMO sounds bit ambiguo Show quoted text On 2018/02/02 17:22:00, a.giammarchi wrote: > On 2018/02/02 16:57:19, saroyanm wrote: > > Nit: You call this element "custom-filters-edit-error" and the list item > > "custom-filters-error". > > > > I don't understand why the one called "edit" when the other one is not, even > oth > > only appear in edit mode. > > > > So I'll suggest to rename this to something more descriptive. ex.: > > custom-filters-error-message > > I think this is very subjective. This is an error about editing within the edit > area so I prefer current id. I see. In general edit-area IMO sounds bit ambiguous in this case, because textarea can also be considered as an edit-area. Currently we use "controls" (ex.: ".side-controls"), to define the form control elements container. Ideally we would use same styling to be consistent. Couple of suggestions for "custom-filters-edit-area": * .control * #custom-filters-control
306 </button>	309 <div class="side-controls">

307 <div class="side-controls" id="custom-filters-raw-controls">	310 <button id="custom-filters-edit" class="i18n_options_customFilte r_edit secondary" data-action="edit-custom-filters">

308 <button class="i18n_options_customFilter_cancel secondary" data- action="cancel-custom-filters"></button>	311 </button>

309 <button class="i18n_options_customFilter_save primary" data-acti on="save-custom-filters"></button>	312 <div id="custom-filters-raw-controls" class="side-controls">

	313 <button class="i18n_options_customFilter_cancel secondary" dat a-action="cancel-custom-filters"></button>

	314 <button class="i18n_options_customFilter_save primary" data-ac tion="save-custom-filters"></button>

	315 </div>

310 </div>	316 </div>

311 </div>	317 </div>

	318 <ul id="custom-filters-error"></ul>
	saroyanm 2018/02/02 16:57:20 While we don't have yet plan simplifying the seman While we don't have yet plan simplifying the semantics, we can improve the accessibility. Suggestion#2: <ul aria-labelledby="custom-filters-edit-error" role="alert" id="custom-filters-error"></ul> This will help assisting technologies, to specify the relationship between the error message and filters and inform the user when the issue will appear also this will help us meet WCAG2 spec requirement. a.giammarchi 2018/02/02 17:22:00 I like that, will do. Show quoted text On 2018/02/02 16:57:20, saroyanm wrote: > While we don't have yet plan simplifying the semantics, we can improve the > accessibility. > Suggestion#2: > <ul aria-labelledby="custom-filters-edit-error" role="alert" > id="custom-filters-error"></ul> > > This will help assisting technologies, to specify the relationship between the > error message and filters and inform the user when the issue will appear also > this will help us meet WCAG2 spec requirement. I like that, will do.
312 </div>	319 </div>

313 </section>	320 </section>

314 </div>	321 </div>

315	322

316 <!-- Help tab content -->	323 <!-- Help tab content -->

317 <div id="content-help" role="tabpanel" aria-labelledby="tab-help">	324 <div id="content-help" role="tabpanel" aria-labelledby="tab-help">

318 <header>	325 <header>

319 <h1 class="i18n_options_tab_help"></h1>	326 <h1 class="i18n_options_tab_help"></h1>

320 <p class="i18n_options_help_description"></p>	327 <p class="i18n_options_help_description"></p>

321 </header>	328 </header>

(...skipping 107 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
429 </div>	436 </div>

430 <!-- Notification -->	437 <!-- Notification -->

431 <div id="notification" aria-hidden="true" aria-live="polite">	438 <div id="notification" aria-hidden="true" aria-live="polite">

432 <strong id="notification-text"></strong>	439 <strong id="notification-text"></strong>

433 <button class="icon close secondary" data-action="hide-notification">	440 <button class="icon close secondary" data-action="hide-notification">

434 <span class="i18n_options_notification_hide sr-only"></span>	441 <span class="i18n_options_notification_hide sr-only"></span>

435 </button>	442 </button>

436 </div>	443 </div>

437 </body>	444 </body>

438 </html>	445 </html>

OLD	NEW

« no previous file with comments | « background.js ('k') | desktop-options.js » ('j') | desktop-options.js » ('J')