whip the feedback page into shape

chrome/browser/resources/feedback.js

Index: chrome/browser/resources/feedback.js
diff --git a/chrome/browser/resources/feedback.js b/chrome/browser/resources/feedback.js
index d2cba0895457955bcc7f2ad29f2f520efb3c3a2e..2d701d6e0a8fbdd0cf04317a865c7af61e4e190b 100644
--- a/chrome/browser/resources/feedback.js
+++ b/chrome/browser/resources/feedback.js
@@ -16,8 +16,6 @@ savedThumbnailIds['saved-screenshots'] = '';
 
 var categoryTag = '';
 
-var localStrings = new LocalStrings();
-
 /**
  * Selects an image thumbnail in the specified div.
  * @param {string} divId The id of the div to search in.
@@ -30,15 +28,16 @@ function selectImage(divId, thumbnailId) {
     $(divId).hidden = true;
     return;
   }
+
   for (var i = 0; i < thumbnailDivs.length; i++) {
+    thumbnailDivs[i].className = 'image-thumbnail-container';

[Dan Beam, 2012/05/16 22:38:14]

you're setting [className] to intentionally nuke the previous (possibly
selected) class, right? does this have any artifacts when the same image is
selected if we use transitions?

[Evan Stade, 2012/05/16 23:56:24]

yes
i doubt it, but we don't use transitions.

+
     // If the the current div matches the thumbnail id provided,
     // or there is no thumbnail id given, and we're at the first thumbnail.
     if ((thumbnailDivs[i].id == thumbnailId) || (!thumbnailId && !i)) {

[Dan Beam, 2012/05/16 22:38:14]

remove first set of ()

[Evan Stade, 2012/05/16 23:56:24]

Done.

-      thumbnailDivs[i].className = 'image-thumbnail-container-selected';
+      thumbnailDivs[i].classList.add('image-thumbnail-container-selected');

[Dan Beam, 2012/05/16 22:38:14]

I assume you're against simply making this .selected so you could say:

  .image-thumbnail-container.selected:not(:only-of-type) {
    border-color: green;
  }

as the class is too generic (and could collide). Or simply changed to [selected]
like we do in shared lists (IIRC)?

[Evan Stade, 2012/05/16 23:56:24]

yes

[Dan Beam, 2012/05/17 05:09:05]

what about second option - [selected]? you're against both? (I understand it was
a poorly worded question)

[Evan Stade, 2012/05/17 16:38:57]

I don't see how selected as an attribute rather than class  gets around the
problem. I don't see much of a drawback to having this long classname except
that it causes an unfortunate line wrap which is really not the end of the
world.

[Dan Beam, 2012/05/17 17:14:37]

I think it's just less common for somebody to have a generic [selected]
selector, but is does have a similar issue (but again, I feel like we should
just be getting rid of this problem with namespacing, i.e. .module
node[selected] vs. something as silly as just [selected]).

If you're OK with the really long class name, that's fine.

       selectedThumbnailId = thumbnailId;
       savedThumbnailIds[divId] = thumbnailId;
-    } else {
-      thumbnailDivs[i].className = 'image-thumbnail-container';
     }
   }
 }
@@ -58,10 +57,7 @@ function addScreenshot(divId, screenshot) {
   };
 
   var innerDiv = document.createElement('div');
-  if (divId == 'current-screenshots')

[Dan Beam, 2012/05/16 22:38:14]

so are you handling this somewhere else or was this functionality deleted?

[Evan Stade, 2012/05/16 23:56:24]

i don't think this really did anything useful and you can see that I deleted the
CSS which referenced image-thumbnail-current

[Dan Beam, 2012/05/17 17:14:37]

OK.

-    innerDiv.className = 'image-thumbnail-current';
-  else
-    innerDiv.className = 'image-thumbnail';
+  innerDiv.className = 'image-thumbnail';
 
   var thumbnail = document.createElement('img');
   thumbnail.id = thumbnailDiv.id + '-image';
@@ -97,7 +93,7 @@ function disableScreenshots() {
  */
 function sendReport() {
   if ($('description-text').value.length == 0) {
-    alert(localStrings.getString('no-description'));
+    alert(loadTimeData.getString('no-description'));
     return false;
   }
 
@@ -157,19 +153,17 @@ function currentSelected() {
  * selected when we had this div open previously.
  */
 function savedSelected() {
-  $('current-screenshots').hidden = true;
-
   if ($('saved-screenshots').childElementCount == 0) {
     // setupSavedScreenshots will take care of changing visibility
     chrome.send('refreshSavedScreenshots');
   } else {
+    $('current-screenshots').hidden = true;
     $('saved-screenshots').hidden = false;
     if (selectedThumbnailDivId != 'saved-screenshots')
       selectImage('saved-screenshots', savedThumbnailIds['saved-screenshots']);
   }
 }
 
-
 /**
  * Change the type of screenshot we're showing to the user from
  * the current screenshot to saved screenshots
@@ -267,9 +261,10 @@ function setupCurrentScreenshot(screenshot) {
 function setupSavedScreenshots(screenshots) {
   if (screenshots.length == 0) {
     $('saved-screenshots').textContent =
-        localStrings.getString('no-saved-screenshots');
+        loadTimeData.getString('no-saved-screenshots');
 
     // Make sure we make the display the message.
+    $('current-screenshots').hidden = true;
     $('saved-screenshots').hidden = false;
 
     // In case the user tries to send now; fail safe, do not send a screenshot