[Toaster] [review-request][PATCH] 8126 Messages are missing when "IMAGE_FSTYPES" field is not properly edited
Barros Pena, Belen
belen.barros.pena at intel.com
Wed Sep 30 04:04:02 PDT 2015
On 30/09/2015 11:59, "Michael Wood" <michael.g.wood at intel.com> wrote:
>On 28/09/15 11:11, Barros Pena, Belen wrote:
>> Sorry for the delay in looking at this.
>>
>> On 23/09/2015 10:02, "toaster-bounces at yoctoproject.org on behalf of
>>Reyna,
>> David" <toaster-bounces at yoctoproject.org on behalf of
>> david.reyna at windriver.com> wrote:
>>
>>> Hi Michael,
>>>
>>> I have made the requested changes and updated
>>> "dreyna/project_fstypes_8126", except for this one:
>>>
>>>> + // 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
>>> Belén's design had this message appear within the selection list when
>>> there were no search matches instead of in the optional error message
>>> below it. That is why I did this implementation. I do toggle it with
>>> .hide .show.
>> FWIW, I agree with David on this one. The message has to be as close as
>> possible to the context of the action. Having the 'no image types found'
>> next to the buttons would be a bit too far away.
>>
>> The branch looks good from the UI standpoint.
>
>Oh I was going on this design:
>http://www.yoctoproject.org/toaster/localconf.html
Ah, I see what you mean know. The prototype reflects the changes that will
be brought in by
https://bugzilla.yoctoproject.org/show_bug.cgi?id=7828
But David is working on the state before that one. Sorry for the confusion.
Belén
>
>
>>
>> Cheers
>>
>> Belén
>>
>>> - David
>>>
>>>> -----Original Message-----
>>>> From: toaster-bounces at yoctoproject.org [mailto:toaster-
>>>> bounces at yoctoproject.org] On Behalf Of Michael Wood
>>>> Sent: Monday, September 14, 2015 7:19 AM
>>>> To: 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/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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> _______________________________________________
>>>> toaster mailing list
>>>> toaster at yoctoproject.org
>>>> https://lists.yoctoproject.org/listinfo/toaster
>>> --
>>> _______________________________________________
>>> toaster mailing list
>>> toaster at yoctoproject.org
>>> https://lists.yoctoproject.org/listinfo/toaster
>
More information about the toaster
mailing list