Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: new-options.js

Issue 29519650: Issue 5540 - implement notification (Closed)
Patch Set: Created Aug. 23, 2017, 6:55 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -432,6 +432,11 @@
{
filter.title = match[1];
collections.whitelist.addItem(filter);
+ if (isCustomFiltersLoaded)
+ {
+ let text = getMessage("options_whitelist_notification", [filter.title]);
+ showNotification(text);
+ }
}
else
{
@@ -588,6 +593,9 @@
case "edit-custom-filters":
setCustomFiltersView("write");
break;
+ case "hide-notification":
+ hideNotification();
+ break;
case "import-subscription": {
let url = E("blockingList-textbox").value;
addEnableSubscription(url);
@@ -993,6 +1001,18 @@
focusedBeforeDialog.focus();
}
+ function showNotification(text)
+ {
+ E("notification").setAttribute("aria-hidden", false);
ire 2017/08/24 10:08:44 The aria-live attribute could also be relevant her
saroyanm 2017/08/24 14:18:25 Agree
saroyanm 2017/08/24 18:40:54 Done.
+ E("notification-text").textContent = text;
+ setTimeout(hideNotification, 3000);
ire 2017/08/24 10:08:43 (Personal Opinion) I think 3 seconds is too short
saroyanm 2017/08/24 14:18:26 That's specified in the specs, let's be consistent
ire 2017/08/25 09:59:44 Acknowledged.
+ }
+
+ function hideNotification()
+ {
+ E("notification").setAttribute("aria-hidden", true);
ire 2017/08/24 10:08:44 NIT: Clear the textContent as well?
saroyanm 2017/08/24 14:18:26 Yes, I'll update that.
saroyanm 2017/08/24 18:40:54 Done.
+ }
+
function setAcceptableAds()
{
let option = "none";

Powered by Google App Engine
This is Rietveld