[Ovirt-devel] [PATCH server] add collapsable sections to vm form
Jason Guiditta
jguiditt at redhat.com
Thu Jul 23 18:15:20 UTC 2009
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, 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. Feel free to apply, make your changes and commit
--amend, I don't care about getting credit :) And sorry for revamping
it so much, it seemed easier to show than to try and explain some of
these issues.
> ---
> 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)
> +
> +
> def radio_button_tag_with_label(label, name, value = "1", checked = false)
> %{
> <div class="i"><label for="#{name}">#{_(label)}</label>
> diff --git a/src/app/views/vm/_form.rhtml b/src/app/views/vm/_form.rhtml
> index 034c3df..373452d 100644
> --- a/src/app/views/vm/_form.rhtml
> +++ b/src/app/views/vm/_form.rhtml
> @@ -6,15 +6,23 @@
> <%= hidden_field 'vm', 'vm_resource_pool_id' %>
> <%= hidden_field_tag 'hardware_pool_id', @hardware_pool.id if @hardware_pool %>
>
> + <div class="form_heading">
> + <%= link_to "", "#", :id => "vm_general_section_link" %>
> + </div>
This is one of the bits revamped in the patch. First, it only needs to
be a div w/js behavior attached. Conceptually, this is the right idea -
header section/body section, but the content does not need to be
injected via js, and the link is unneeded in this case. Second, at
least in ff3.5, clicking the link would cause the page to jump down
(need to use the jq preventDefault() method on the anchor, as we do in
_tree.rhtml)
> + <div id="vm_general_config">
> <%= text_field_with_label "Name:", "vm", "description", {:style=>"width:250px;"} %>
> <%= text_field_with_label "UUID:", "vm", "uuid", {:style=>"width:250px;"} %>
> <%= select_with_label "Operating System:", 'vm', 'provisioning_and_boot_settings', @provisioning_options, :style=>"width:250px;" %>
> <% if controller.action_name == "edit" %><b style="color: #FF0000">*Warning* Editing provision could overwrite vm</b><% end %>
> +
> <div class="clear_row" style="height:15px;"></div>
> + </div>
> + <div class="clear_row"></div>
>
> - <div class="form_heading">Resources</div>
> - <div class="clear_row"></div>
> - <div class="clear_row"></div>
> + <div class="form_heading">
> + <%= link_to "", "#", :id => "vm_resources_section_link" %>
> + </div>
> + <div id="vm_resources_config">
> <div style="float:left;width:150px;" >
> <%= text_field_with_label "CPUs:", "vm", "num_vcpus_allocated", {:style=>"width:100px; margin-bottom:2px;"}, {:style=>"padding-right: 50px;"} %>
> <div class="field_helptext">max to create: <%=create_resources[:cpus]%> </div>
> @@ -27,6 +35,13 @@
> </div>
> <div style="clear:both;"></div>
> <div class="clear_row"></div>
> + </div>
> + <div class="clear_row"></div>
> +
> + <div class="form_heading">
> + <%= link_to "", "#", :id => "vm_storage_section_link" %>
> + </div>
> + <div id="vm_storage_config">
> <div class="field_title">Storage: </div>
> <div style="height:150px; overflow:auto; border:#CCCCCC solid 1px;">
> <ul id="storage_volumes_tree" class="ovirt-tree"></ul>
> @@ -35,10 +50,13 @@
> <!-- FIXME: fill in total here -->
> <div style="background:#F3F3F3; padding:6px; border-left:#CCCCCC solid 1px; border-right:#CCCCCC solid 1px; border-bottom:#CCCCCC solid 1px; ">Total:</div>
> <div class="clear_row" style="height:15px;"></div>
> - <div class="clear_row"></div>
> - <div class="clear_row"></div>
> + </div>
> + <div class="clear_row"></div>
>
> - <div class="form_heading">Network</div>
> + <div class="form_heading">
> + <%= link_to "", "#", :id => "vm_network_section_link" %>
> + </div>
> + <div id="vm_network_config">
> <div class="clear_row"></div>
> <div class="clear_row"></div>
> <div style="float:left;">
> @@ -47,15 +65,14 @@
> <div style="float:left;">
> <%= select_with_label "Network:", 'vm', 'network_id', @networks.insert(0, ""), :style=>"width:250px;" %>
> </div>
> - <div style="clear:both;"></div>
> - <div class="clear_row"></div>
> <div class="clear_row"></div>
>
> - <%= check_box_tag_with_label "Forward vm's vnc port locally", "forward_vnc", 1, @vm.forward_vnc %>
> - <div class="clear_row"></div>
> + <%= rcheck_box_tag_with_label "Forward vm's vnc port locally", "forward_vnc", 1, @vm.forward_vnc %>
> + </div>
> + <div class="clear_row"></div>
>
> - <%= check_box_tag_with_label "Start VM Now? (pending current resource availability)", "start_now", nil if create or @vm.state == Vm::STATE_STOPPED %>
> - <%= check_box_tag_with_label "Restart VM Now? (pending current resource availability)", "restart_now", nil if @vm.state == Vm::STATE_RUNNING %>
> + <%= rcheck_box_tag_with_label "Start VM Now? (pending current resource availability)", "start_now", nil if create or @vm.state == Vm::STATE_STOPPED %>
> + <%= rcheck_box_tag_with_label "Restart VM Now? (pending current resource availability)", "restart_now", nil if @vm.state == Vm::STATE_RUNNING %>
>
> <!--[eoform:vm]-->
>
> @@ -104,6 +121,18 @@ ${htmlList(pools, id)}
> refresh: VmCreator.returnToVmForm
> });
> });
> -</script>
>
> + toggle_visability_on_click('#vm_general_config', '#vm_general_section_link', 'General');
> + toggle_visability_on_click('#vm_resources_config', '#vm_resources_section_link', 'Resources');
> + toggle_visability_on_click('#vm_storage_config', '#vm_storage_section_link', 'Storage');
> + toggle_visability_on_click('#vm_network_config', '#vm_network_section_link', 'Network');
> +
> + // initially show general / resources, hide storage / networks section
> + $(document).ready(function(){
> + show_section_with_header('#vm_general_config', '#vm_general_section_link', 'General');
> + show_section_with_header('#vm_resources_config', '#vm_resources_section_link', 'Resources');
> + hide_section_with_header('#vm_storage_config', '#vm_storage_section_link', 'Storage');
> + hide_section_with_header('#vm_network_config', '#vm_network_section_link', 'Network');
> + });
>
This bit ^^ is all ripped/replaced in the attached patch with a much
shorter way to get the same effect. Chaining ftw!
> +</script>
> diff --git a/src/public/javascripts/ovirt.js b/src/public/javascripts/ovirt.js
> index 67dc455..6055e53 100644
> --- a/src/public/javascripts/ovirt.js
> +++ b/src/public/javascripts/ovirt.js
> @@ -394,3 +394,28 @@ function get_server_from_url()
> var end = window.location.href.indexOf('/', 8) - start;
> return window.location.href.substr(start, end);
> }
> +
> +// hides the specified section, altering the specified header div with updated title / arrow
> +function hide_section_with_header(section, header, title){
> + content = '<img src="/ovirt/images/dir_closed.png" /><div>' + title + '</div>';
Not a big deal, since I moved this into a css class anyway, but this
path should not start w/ovirt, as if you run ovirt outside our default
context (say give it another name in apache, or run straight mongrel),
the image cannot be found by the browser.
> + $(header).html(content);
> + $(section).hide('slow');
> +};
> +
> +// show the specified section, altering the specified header div with updated title / arrow
> +function show_section_with_header(section, header, title){
> + content = '<img src="/ovirt/images/dir_open.png" /><div>' + title + '</div>';
> + $(header).html(content);
> + $(section).show('slow');
> +};
> +
> +// wire up the header to invoke either the show or hide function on click
> +function toggle_visability_on_click(section, header, title){
> + $(header).bind('click', function(){
> + if($(section).is(':hidden')){
> + show_section_with_header(section, header, title);
> + }else{
> + hide_section_with_header(section, header, title);
> + }
> + });
> +};
All the above changes can be backed out.
> diff --git a/src/public/stylesheets/components.css b/src/public/stylesheets/components.css
> index 41ad3d0..1409692 100644
> --- a/src/public/stylesheets/components.css
> +++ b/src/public/stylesheets/components.css
> @@ -339,3 +339,18 @@
> height: 11px;
> }
>
> +#vm_general_config, #vm_resources_config, #vm_storage_config, #vm_network_config{
> + padding-left: 30px;
> +}
> +
> +#vm_general_section_link img, #vm_resources_section_link img, #vm_storage_section_link img, #vm_network_section_link img{
> + float: left;
> + padding-top: 2px;
> +}
> +
> +#vm_general_section_link div, #vm_resources_section_link div, #vm_storage_section_link div, #vm_network_section_link div{
> + padding-top: 3px;
> + padding-left: 20px;
> + padding-bottom: 3px;
> + width: 25%;
> +}
Likewise, these are not needed, just one class (in patch).
> --
> 1.6.0.6
>
> _______________________________________________
> Ovirt-devel mailing list
> Ovirt-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/ovirt-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Revamp-suggestions-cleanup.patch
Type: text/x-patch
Size: 4739 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/ovirt-devel/attachments/20090723/fff2d9fc/attachment.bin>
More information about the ovirt-devel
mailing list