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

Delta Between Two Patch Sets: desktop-options.html

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

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

Right Patch Set: changed custom-filters-edit-area to custom-filters-control as suggested Created Feb. 5, 2018, 9:05 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-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 282 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
293 <a class="i18n_options_customFilters_learn" id="link-filters-1" ta rget="_blank"></a>	293 <a class="i18n_options_customFilters_learn" id="link-filters-1" ta rget="_blank"></a>

294 </p>	294 </p>

295 </header>	295 </header>

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

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

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

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

300 <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">

301 </button>	301 </button>

302 <p>	302 <p>

303 <a class="i18n_options_customFilters_learn" id="link-filters-2" target="_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.
304 </p>	304 </p>

305 </div>	305 </div>

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

307 <div id="custom-filters-edit-area">	307 <div id="custom-filters-control">

308 <div id="custom-filters-edit-error" class="i18n_options_customFilt ers_edit_error"></div>	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
309 <div class="side-controls">	309 <div class="side-controls">

310 <button id="custom-filters-edit" class="i18n_options_customFilte r_edit secondary" data-action="edit-custom-filters">	310 <button id="custom-filters-edit" class="i18n_options_customFilte r_edit secondary" data-action="edit-custom-filters">

311 </button>	311 </button>

312 <div id="custom-filters-raw-controls" class="side-controls">	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>	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>	314 <button class="i18n_options_customFilter_save primary" data-ac tion="save-custom-filters"></button>

315 </div>	315 </div>

316 </div>	316 </div>

317 </div>	317 </div>

318 <ul id="custom-filters-error"></ul>	318 <ul aria-labelledby="custom-filters-edit-error" role="alert" id="cus tom-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.
319 </div>	319 </div>

320 </section>	320 </section>

321 </div>	321 </div>

322	322

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

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

325 <header>	325 <header>

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

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

328 </header>	328 </header>

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

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

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

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

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

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

442 </button>	442 </button>

443 </div>	443 </div>

444 </body>	444 </body>

445 </html>	445 </html>

LEFT	RIGHT