new-options.html - Issue 29519650: Issue 5540 - implement notification

Side by Side Diff

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

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

Side by Side Diff: new-options.html

Issue 29519650: Issue 5540 - implement notification (Closed)

Patch Set: Created Aug. 23, 2017, 6:55 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 356 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
367 <div class="dialog-content-block">	367 <div class="dialog-content-block">

368 <h3></h3>	368 <h3></h3>

369 <div class="url"></div>	369 <div class="url"></div>

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

371 </div>	371 </div>

372 </div>	372 </div>

373 </div>	373 </div>

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

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

376 </div>	376 </div>

	377 <!-- Notification -->

	378 <div id="notification" aria-hidden="true">

	379 <span id="notification-text"></span>

	380 <button data-action="hide-notification" id="hide-notification"></button>
	ire 2017/08/24 10:08:43 This button doesn't have an accessible name This button doesn't have an accessible name ire 2017/08/24 10:08:43 It's part of our coding style to not omit optional It's part of our coding style to not omit optional tags, so there should be a `type="button"` on this saroyanm 2017/08/24 14:18:25 Good point, I'll use aria-label="Close" Show quoted text On 2017/08/24 10:08:43, ire wrote: > This button doesn't have an accessible name Good point, I'll use aria-label="Close" saroyanm 2017/08/24 14:18:25 I think we were overriding current -> https://goog Show quoted text On 2017/08/24 10:08:43, ire wrote: > It's part of our coding style to not omit optional tags, so there should be a > `type="button"` on this I think we were overriding current -> https://google.github.io/styleguide/htmlcssguide.html#Optional_Tags Google style, which was about the HTML tags, but not attributes. In general I agree that buttons here should specify types, currently all of them are missing type attribute, so I think it's better to be consistent in that case, things probably will change as soon we will start implementing -> https://issues.adblockplus.org/ticket/5251 so probably that will make sense to cover that there as well. saroyanm 2017/08/24 18:40:53 Done. Show quoted text On 2017/08/24 14:18:25, saroyanm wrote: > On 2017/08/24 10:08:43, ire wrote: > > This button doesn't have an accessible name > > Good point, I'll use aria-label="Close" Done. ire 2017/08/25 09:59:44 Okay, that makes sense. Show quoted text On 2017/08/24 14:18:25, saroyanm wrote: > On 2017/08/24 10:08:43, ire wrote: > > It's part of our coding style to not omit optional tags, so there should be a > > `type="button"` on this > > I think we were overriding current -> > https://google.github.io/styleguide/htmlcssguide.html#Optional_Tags Google > style, which was about the HTML tags, but not attributes. > > In general I agree that buttons here should specify types, currently all of them > are missing type attribute, so I think it's better to be consistent in that > case, things probably will change as soon we will start implementing -> > https://issues.adblockplus.org/ticket/5251 so probably that will make sense to > cover that there as well. Okay, that makes sense.
	381 </div>

377 </body>	382 </body>

378 </html>	383 </html>

OLD	NEW

« locale/en-US/new-options.json ('K') | « locale/en-US/new-options.json ('k') | new-options.js » ('j') | new-options.js » ('J')