fix code review comments: hide advanced options by default, add missing semi-colon, remove duplicate TODO

vijayp · vijayp · commit ee74c9fbfd1f · 2014-01-12T16:41:46.000-05:00
diff --git a/chromium/background.js b/chromium/background.js
@@ -2,7 +2,7 @@ var USER_RULE_KEY = 'userRules';
 
 var getStoredUserRules = function() {
   var oldUserRuleString = localStorage.getItem(USER_RULE_KEY);
-  var oldUserRules = []
+  var oldUserRules = [];
   if (oldUserRuleString) {
     oldUserRules = JSON.parse(oldUserRuleString);
   }
diff --git a/chromium/popup.html b/chromium/popup.html
@@ -19,10 +19,20 @@ <h1 i18n="about_ext_name"></h1>
   <a href="#" id="add-rule-link">Add this site</a>
   <div id="add-new-rule-div" style="display:none">
     <h3> Add a new rule</h3>
+    <p>Always use https for this host.
     <label for="new-rule-host">Host</label> <br><input size="50" id="new-rule-host" type="text" disabled><br>
-    <label for="new-rule-name">Rule name</label><br><input size="50" id="new-rule-name" type="text"><br>
-    <label for="new-rule-regex">Matching regex</label> <br><input size="50" id="new-rule-regex" type="text"><br>
-    <label for="new-rule-redirect">Redirect to</label> <br><input size="50" id="new-rule-redirect" type="text"><br>
+
+    <div id="new-rule-regular-text">
+      <p><a href="#" id="new-rule-show-advanced-link">Show advanced</a>
+    </div>
+    <div id="new-rule-advanced" style="display:none;">
+      <p><a href="#" id="new-rule-hide-advanced-link">Hide advanced</a>
+      <p>
+      <label for="new-rule-name">Rule name</label><br><input size="50" id="new-rule-name" type="text"><br>
+      <label for="new-rule-regex">Matching regex</label> <br><input size="50" id="new-rule-regex" type="text"><br>
+      <label for="new-rule-redirect">Redirect to</label> <br><input size="50" id="new-rule-redirect" type="text"><br>
+    </div>
+
     <button id="add-new-rule-button">Add new Rule</button><br>
     <button id="cancel-new-rule">Cancel</button>
   </div>
diff --git a/chromium/popup.js b/chromium/popup.js
@@ -126,7 +126,7 @@ function addManualRule() {
       var params = {
         host : e("new-rule-host").value,
         redirectTo : e("new-rule-redirect").value,
-        urlMatcher : e("new-rule-regex").value,
+        urlMatcher : e("new-rule-regex").value
       };
       backgroundPage.addNewRule(params, function() {
         location.reload();
@@ -137,5 +137,13 @@ function addManualRule() {
       show(e("add-rule-link"));
       hide(e("add-new-rule-div"));
     });
+    e("new-rule-show-advanced-link").addEventListener("click", function() {
+      show(e("new-rule-advanced"));
+      hide(e("new-rule-regular-text"));
+    });
+    e("new-rule-hide-advanced-link").addEventListener("click", function() {
+      hide(e("new-rule-advanced"));
+      show(e("new-rule-regular-text"));
+    });
   });
-};
+}
diff --git a/chromium/rules.js b/chromium/rules.js
@@ -126,7 +126,6 @@ RuleSets.prototype = {
     }
     ruleCache.remove(params.host);
     // TODO: maybe promote this rule?
-    // TODO: look for duplicates.
     this.targets[params.host].push(new_rule_set);
     log(INFO, 'done adding rule');
     return true;

Original file line number	Diff line number	Diff line change
`@@ -2,7 +2,7 @@ var USER_RULE_KEY = 'userRules';`
`2`	`2`
`3`	`3`	`var getStoredUserRules = function() {`
`4`	`4`	`var oldUserRuleString = localStorage.getItem(USER_RULE_KEY);`
`5`		`- var oldUserRules = []`
	`5`	`+ var oldUserRules = [];`
`6`	`6`	`if (oldUserRuleString) {`
`7`	`7`	`oldUserRules = JSON.parse(oldUserRuleString);`
`8`	`8`	`}`
Original file line number	Diff line number	Diff line change
`@@ -126,7 +126,6 @@ RuleSets.prototype = {`
`126`	`126`	`}`
`127`	`127`	`ruleCache.remove(params.host);`
`128`	`128`	`// TODO: maybe promote this rule?`
`129`		`- // TODO: look for duplicates.`
`130`	`129`	`this.targets[params.host].push(new_rule_set);`
`131`	`130`	`log(INFO, 'done adding rule');`
`132`	`131`	`return true;`