[Toaster] [review-request][PATCH] 8126 Messages are missing when "IMAGE_FSTYPES" field is not properly edited

[Michael Wood]

On 02/09/15 16:58, Barros Pena, Belen wrote:
>
> On 02/09/2015 15:46, "Reyna, David" <david.reyna at windriver.com> wrote:
>
>> Hi Belén,
>>
>> Thank you for the observation.
>>
>> I have updated "dreyna/project_fstypes_8126" to address that issue, and
>> the code now always pre-initializes the warning message element so that
>> the previous value is not dangling.
> yep: this seems to behave as expected now. Thanks!
>
> I've also realised that the base branch seems a bit old. Could you rebase
> and resubmit?
>
> Cheers
>
> Belén
>
>> - David
>>
>>
>>> -----Original Message-----
>>> From: Barros Pena, Belen [mailto:belen.barros.pena at intel.com]
>>> Sent: Wednesday, September 02, 2015 2:16 AM
>>> To: Reyna, David
>>> Cc: toaster at yoctoproject.org
>>> Subject: Re: [Toaster] [review-request][PATCH] 8126 Messages are missing
>>> when "IMAGE_FSTYPES" field is not properly edited
>>>
>>>
>>>
>>> On 02/09/2015 08:44, "Reyna, David" <david.reyna at windriver.com> wrote:
>>>
>>>> Hi Belén,
>>>>
>>>> Please find the patch for 8126 here:
>>>>
>>>>    dreyna/project_fstypes_8126
>>> Hi David,
>>>
>>> This is looking fairly good. I've only run across one problem. This is
>>> how
>>> to reproduce:
>>>
>>> 1. Click the 'change' icon for IMAGE_FSTYPES
>>>
>>> 2. Deselect all values: the 'save' button becomes disabled and the
>>> message
>>> asking you to select at least one image type appears. This is the
>>> expected
>>> behaviour
>>>
>>> 3. Now, click the 'cancel' link. The IMAGE_FSTYPES value stays the way
>>> it
>>> was before you clicked the 'change' icon. This is once more the correct
>>> behaviour
>>>
>>> 4. Now click the 'change' icon again. There are image types selected,
>>> but
>>> the message 'You must select at least one image type' still shows, and
>>> the
>>> 'Save' button is disabled. This is not the correct behaviour. As long as
>>> there is at least one checkbox ticked you should see no message and the
>>> 'save' button should be enabled. If you make a change (untick a box),
>>> the
>>> validation kicks in and things return to the correct state. Sounds like
>>> we
>>> need to check the selected values whenever the 'change' icon is clicked
>>>
>>> Thanks!
>>>
>>> Belén
>>>
>>>> Note: for the message 'label' the I insert and then show when there are
>>>> no matches, it is guaranteed not to pollute the database because it can
>>>> never be in the checked state.
>>>>
>>>> - David
>>>>

Some review comments below:

 From bac104e20efc46d4746af58aebb0f920c37edfc3 Mon Sep 17 00:00:00 2001
From: David Reyna <David.Reyna at windriver.com>
Date: Wed, 2 Sep 2015 15:34:14 -0700
Subject: bitbake: toaster: display warnings for bad "IMAGE_FSTYPES" values

Display warning message for IMAGE_FSTYPES when no value is selected or
when the filter does not have any matches

[YOCTO #8126]

Signed-off-by: David Reyna <David.Reyna at windriver.com>

diff --git a/bitbake/lib/toaster/toastergui/templates/projectconf.html b/bitbake/lib/toaster/toastergui/templates/projectconf.html
index 4c5a188..b477b4e 100644
--- a/bitbake/lib/toaster/toastergui/templates/projectconf.html
+++ b/bitbake/lib/toaster/toastergui/templates/projectconf.html
@@ -43,6 +43,7 @@
                      <input id="filter-image_fstypes" type="text" placeholder="Search image types" class="span4">
                      <div id="all-image_fstypes" class="scrolling">
                      </div>
+                    <span class="help-block" id="fstypes-error-message"></span>
                      <button id="apply-change-image_fstypes" type="button" class="btn">Save</button>
                      <button id="cancel-change-image_fstypes" type="button" class="btn btn-link">Cancel</button>
                  </form>
@@ -312,9 +313,11 @@
              });
              if ( 0 == any_checked ) {

		 $("#apply-change-image_fstypes").attr("disabled","disabled");
+                $('#fstypes-error-message').html("You must select at least one image type");

MW - Given that this error message doesn't change from this text, rather than adding and removing the text add the text to the DOM in the #fstypes-error-message element and .hide .show this makes it easier to update the error message in the html and keeps a slightly cleaner separation between the UI description and the logic of the javascript.

	     }
              else {
                  $("#apply-change-image_fstypes").removeAttr("disabled");
+                $('#fstypes-error-message').html("");
              }
          }
  
@@ -546,10 +549,14 @@
                  // Add the un-checked boxes second
                  for (var i = 0, length = fstypes_list.length; i < length; i++) {
                      if (0  > fstypes.indexOf(" "+fstypes_list[i].value+" ")) {
-                            html += '<label class="checkbox"><input type="checkbox" class="fs-checkbox-fstypes" value="'+fstypes_list[i].value+'">'+fstypes_list[i].value+'</label>\n';
+                        html += '<label class="checkbox"><input type="checkbox" class="fs-checkbox-fstypes" value="'+fstypes_list[i].value+'">'+fstypes_list[i].value+'</label>\n';
                      }
                  }
+                // Add the 'no search matches' line last
+                html += '<label id="no-match-fstypes">No image types found</label>\n';


MW - Rather than appending this to the list just add the text to the #fstypes-error-message and set it it initially to style="display:none" and then toggle it with .hide .show

+                // Display the list
                  document.getElementById("all-image_fstypes").innerHTML = html;
+                $('#no-match-fstypes').hide();
  
                  // Watch elements to disable Save when none are checked
                  $(".fs-checkbox-fstypes").each(function(){
@@ -558,8 +565,9 @@
                      });
                  });
  
-                // clear the previous filter values
+                // clear the previous filter values and warning messages
                  $("input#filter-image_fstypes").val("");
+                $('#fstypes-error-message').html("");
              });
  
              $('#cancel-change-image_fstypes').click(function(){
@@ -569,17 +577,24 @@
              });
  
              $('#filter-image_fstypes').on('input', function(){
-                   var valThis = $(this).val().toLowerCase();
+                var valThis = $(this).val().toLowerCase();
+                var match_count=0;


MW - camelCase please


		     $('#all-image_fstypes label').each(function(){
                      var text = $(this).text().toLowerCase();
                      var match = text.indexOf(valThis);
                      if (match >= 0) {
                          $(this).show();
+                        match_count += 1;
                      }
                      else {
                          $(this).hide();
                      }
                  });
+                if (0 == match_count) {


MW - This should be if (matchCount === 0)
triple equals for full equality and variable on the left



  +                   $('#no-match-fstypes').show();
+                } else {
+                   $('#no-match-fstypes').hide();
+                }
              });
  
              $('#apply-change-image_fstypes').click(function(){
-- 
cgit v0.10.2