[Ovirt-devel] [PATCH server] add collapsable sections to vm form

[Mohammed Morsi]

Jason Guiditta wrote:
> On Thu, 2009-07-09 at 17:56 -0400, Mohammed Morsi wrote:
>   
>> the vm form is getting cluttered, this patch simply add
>>   collapsable sections to the form, making the 'storage'
>>   and 'network' sections collapsed by default
>>     
>
> Sorry to have to do this, but NACK, 
No need to say sorry, this is what these reviews are for.

> I have a number of issues with this
> patch, outlined in detail inline.  I have attached a patch that you can
> git apply to your local checkout on top of your patch to hopefully save
> you some effort.  This patch redoes the js and gives as an example one
> closed and one open section on the form.  Also contains some cleaned
> up/tweaked css.  
Yes your patch makes things alot cleaner. Thanks alot!

> Feel free to apply, make your changes and commit
> --amend, I don't care about getting credit :)  
I credited you in the commit log anyways :-)


> And sorry for revamping
> it so much, it seemed easier to show than to try and explain some of
> these issues.
>   
No worries, we're all seeking the optimal solution in any case.


>> ---
>>  src/app/helpers/application_helper.rb |    9 +++++
>>  src/app/views/vm/_form.rhtml          |   55 +++++++++++++++++++++++++--------
>>  src/public/javascripts/ovirt.js       |   25 +++++++++++++++
>>  src/public/stylesheets/components.css |   15 +++++++++
>>  4 files changed, 91 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/app/helpers/application_helper.rb b/src/app/helpers/application_helper.rb
>> index 0178ad0..d3eecd7 100644
>> --- a/src/app/helpers/application_helper.rb
>> +++ b/src/app/helpers/application_helper.rb
>> @@ -77,6 +77,15 @@ module ApplicationHelper
>>       }
>>    end
>>  
>> +  # same as check_box_tag_with_label but w/ the checkbox appearing first
>> +  def rcheck_box_tag_with_label(label, name, value = "1", checked = false)
>> +    %{
>> +      <div class="i">#{check_box_tag name, value, checked}
>> +      <label for="#{name}">#{_(label)}</label></div>
>> +     }
>> +  end
>>     
> instead of making a new method here, just change the order this way in
> the existing check_box_tag_with_label method.  I discussed with jeremy
> perry, and checkbox on the left is best practice, so the whole app
> should do this anyway (bonus points if you fix radio buttons the same
> way, that needs to be done as well)
>   
I removed this and changed the checkbox tag to have the checkbox first.
It looks good (as it did b4)
on the vms form but I didn't checkout the rest of the places where this
is to make sure nothing looks
off (though I doubt it will as the same screen space is used w/ either
approach)


Everything else looks good. You missed a couple small things (like the
headers to the storage/resources
sections) but I got it all.

I'm sending this and the updated default_values patches on their own.
I'll look to updating the remaining
patches to work with these next.

Once again thanks alot for your review / changes

  -Mo