[Ovirt-devel] [PATCH] consolidated the storage action links.
Jason Guiditta
jguiditt at redhat.com
Fri Jun 27 15:43:13 UTC 2008
On Wed, 2008-06-18 at 14:09 -0400, Scott Seago wrote:
> Add and Create Storage Pool are now two tabs on the same popup widget. I've also consolidated delete and remove, adding a confirmation dialog for the user to choose which action to take (as specified in the design mockups).
>
> One thing remaining for a follow-on task is to add some css styling for the radio buttons and text to make it conform better to the design mockups.
>
> Signed-off-by: Scott Seago <sseago at redhat.com>
> ---
> wui/src/app/controllers/hardware_controller.rb | 5 ++
> wui/src/app/controllers/storage_controller.rb | 13 ++--
> wui/src/app/helpers/application_helper.rb | 7 ++
> wui/src/app/views/hardware/removestorage.rhtml | 25 +++++++
> wui/src/app/views/hardware/show_storage.rhtml | 21 ++-----
> .../app/views/storage/{new.rhtml => _new.rhtml} | 11 +---
> wui/src/app/views/storage/addstorage.html.erb | 67 +++++++++++---------
> wui/src/public/javascripts/ovirt.js | 27 ++++++++
> wui/src/public/stylesheets/components.css | 3 +
> 9 files changed, 118 insertions(+), 61 deletions(-)
> create mode 100644 wui/src/app/views/hardware/removestorage.rhtml
> rename wui/src/app/views/storage/{new.rhtml => _new.rhtml} (77%)
>
> diff --git a/wui/src/app/controllers/hardware_controller.rb b/wui/src/app/controllers/hardware_controller.rb
> index 9718d60..6d56e31 100644
> --- a/wui/src/app/controllers/hardware_controller.rb
> +++ b/wui/src/app/controllers/hardware_controller.rb
> @@ -290,6 +290,11 @@ class HardwareController < ApplicationController
> end
> end
>
> + def removestorage
> + pre_modify
> + render :layout => 'popup'
> + end
> +
> def destroy
> parent = @pool.parent
> if not(parent)
> diff --git a/wui/src/app/controllers/storage_controller.rb b/wui/src/app/controllers/storage_controller.rb
> index 492c4e1..25121d2 100644
> --- a/wui/src/app/controllers/storage_controller.rb
> +++ b/wui/src/app/controllers/storage_controller.rb
> @@ -81,9 +81,6 @@ class StorageController < ApplicationController
> end
>
> def new
> - @storage_pools = @hardware_pool.storage_volumes
> - @storage_types = StoragePool::STORAGE_TYPES.keys
> - render :layout => 'popup'
> end
>
> def new2
> @@ -145,10 +142,12 @@ class StorageController < ApplicationController
> end
>
> def addstorage
> - @hardware_pool = Pool.find(params[:hardware_pool_id])
> - @unassigned = Pool.root.storage_pools.size
> - # FIXME: @assigned should match the updated assigned storage pools query when that's done
> - @assigned = StoragePool.find(:all).size
> + @hardware_pool = HardwarePool.find(params[:hardware_pool_id])
> + @perm_obj = @hardware_pool
> + @redir_controller = @perm_obj.get_controller
> + authorize_admin
> + @storage_pools = @hardware_pool.storage_volumes
> + @storage_types = StoragePool::STORAGE_TYPES.keys
> render :layout => 'popup'
> end
>
> diff --git a/wui/src/app/helpers/application_helper.rb b/wui/src/app/helpers/application_helper.rb
> index 75b6195..58a6358 100644
> --- a/wui/src/app/helpers/application_helper.rb
> +++ b/wui/src/app/helpers/application_helper.rb
> @@ -77,6 +77,13 @@ module ApplicationHelper
> }
> end
>
> + def radio_button_tag_with_label(label, name, value = "1", checked = false)
> + %{
> + <div class="i"><label for="#{name}">#{_(label)}</label>
> + #{radio_button_tag name, value, checked}</div>
> + }
> + end
> +
> def popup_footer(action, label)
> %{
> <div style="background: url(#{image_path "fb_footer.jpg"}) repeat-x; height: 37px; text-align:right; padding: 9px 9px 0 0;">
> diff --git a/wui/src/app/views/hardware/removestorage.rhtml b/wui/src/app/views/hardware/removestorage.rhtml
> new file mode 100644
> index 0000000..8cee2e7
> --- /dev/null
> +++ b/wui/src/app/views/hardware/removestorage.rhtml
> @@ -0,0 +1,25 @@
> +<%- content_for :title do -%>
> + Remove Storage Pool
> +<%- end -%>
> +<%- content_for :description do -%>
> + Please confirm your choice to remove this storage pool.
> +<%- end -%>
> +
> +<form id="remove_storage_selection" >
> +<div class="dialog_form">
> + <%= error_messages_for 'remove_storage_pool' %>
> +
> + <% form_tag do %>
> + <!--[form:storage_pool]-->
> + <%= hidden_field_tag 'hardware_pool_id', @pool.id %>
> + <% if @pool.id != HardwarePool.get_default_pool.id %>
> + <%= radio_button_tag_with_label "Remove this Storage Pool from #{@pool.name} (moving it to #{HardwarePool.get_default_pool.name})", "remove_selection", "remove", true %>
> + <%= radio_button_tag_with_label "Delete this Storage Pool (making its storage volumes unavailable)", "remove_selection", "delete", false %>
> + <% else %>
> + <%= radio_button_tag_with_label "Delete this Storage Pool (making its storage volumes unavailable)", "remove_selection", "delete", true %>
> + <% end %>
> + <!--[eoform:storage_pool]-->
> + <% end %>
> +</div>
> +</form>
> +<%= popup_footer("delete_or_remove_storage()", "Remove Storage Pool") %>
> diff --git a/wui/src/app/views/hardware/show_storage.rhtml b/wui/src/app/views/hardware/show_storage.rhtml
> index 08c762a..ec1a82d 100644
> --- a/wui/src/app/views/hardware/show_storage.rhtml
> +++ b/wui/src/app/views/hardware/show_storage.rhtml
> @@ -1,29 +1,18 @@
> <div id="toolbar_nav">
> <ul>
> <li><a href="<%= url_for :controller => 'storage', :action => 'addstorage', :hardware_pool_id => @pool %>" rel="facebox[.bolder]"><%= image_tag "icon_addstorage.png", :style => "vertical-align:middle;" %> Add Storage Server</a></li>
> - <li><a href="<%= url_for :controller => 'storage', :action => 'new', :hardware_pool_id => @pool %>" rel="facebox[.bolder]"><%= image_tag "icon_addstorage.png", :style => "vertical-align:middle;" %> Create Storage Server</a></li>
> <li>
> - <a href="#" onClick="return validate_for_move();" ><%= image_tag "icon_move.png", :style=>"vertical-align:middle;" %> Move</a>
> + <a href="#" onClick="return validate_storage_for_move();" ><%= image_tag "icon_move.png", :style=>"vertical-align:middle;" %> Move</a>
> <a id="move_link_hidden" href="<%= url_for :controller => 'hardware', :action => 'move', :id => @pool, :resource_type=>'storage' %>" rel="facebox[.bolder]" style="display:none" ></a>
> </li>
> - <li><a href="#" onClick="remove_storage()"><%= image_tag "icon_remove.png", :style => "vertical-align:middle;" %> Remove</a></li>
> - <% if @pool.id != HardwarePool.get_default_pool.id %>
> - <li><a href="#" onClick="delete_storage()"><%= image_tag "icon_delete_white.png", :style => "vertical-align:middle;" %> Delete</a></li>
> - <% end %>
> + <li>
> + <a href="#" onClick="return validate_storage_for_remove();" ><%= image_tag "icon_remove.png", :style=>"vertical-align:middle;" %> Remove</a>
> + <a id="remove_link_hidden" href="<%= url_for :controller => 'hardware', :action => 'removestorage', :id => @pool %>" rel="facebox[.bolder]" style="display:none" ></a>
> + </li>
> </ul>
> </div>
>
> <script type="text/javascript">
> - function get_selected_storage()
> - {
> - return get_selected_checkboxes("storage_grid_form")
> - }
> - function validate_for_move()
> - {
> - if (validate_selected(get_selected_storage(), 'storage pool')) {
> - $('#move_link_hidden').click()
> - }
> - }
> function remove_storage()
> {
> storage = get_selected_storage()
> diff --git a/wui/src/app/views/storage/new.rhtml b/wui/src/app/views/storage/_new.rhtml
> similarity index 77%
> rename from wui/src/app/views/storage/new.rhtml
> rename to wui/src/app/views/storage/_new.rhtml
> index 13dedb9..a113b67 100644
> --- a/wui/src/app/views/storage/new.rhtml
> +++ b/wui/src/app/views/storage/_new.rhtml
> @@ -1,17 +1,10 @@
> -<%- content_for :title do -%>
> - Add New Storage Pool
> -<%- end -%>
> -<%- content_for :description do -%>
> - Add a new Storage Pool to this oVirt server.
> -<%- end -%>
> -
> <div class="dialog_form">
> <%= error_messages_for 'storage_pool' %>
>
> <% form_tag do %>
> <!--[form:storage_pool]-->
> - <%= hidden_field_tag 'hardware_pool_id', @hardware_pool.id %>
> - <%= select_tag_with_label "Storage Type:", 'storage_type', @storage_types, :onChange => "load_storage_subform()" %>
> + <%= hidden_field_tag 'hardware_pool_id', hardware_pool.id %>
> + <%= select_tag_with_label "Storage Type:", 'storage_type', storage_types, :onChange => "load_storage_subform()" %>
> <!--[eoform:storage_pool]-->
> <% end %>
>
> diff --git a/wui/src/app/views/storage/addstorage.html.erb b/wui/src/app/views/storage/addstorage.html.erb
> index fa49111..11e9135 100644
> --- a/wui/src/app/views/storage/addstorage.html.erb
> +++ b/wui/src/app/views/storage/addstorage.html.erb
> @@ -1,42 +1,51 @@
> <%- content_for :title do -%>
> - Add Storage
> + Add Storage Pool
> <%- end -%>
> <%- content_for :description do -%>
> - Select storage pools from the list below to add to the <%= @hardware_pool.name %> hardware pool.
> + Choose a storage pool selection below
> <%- end -%>
>
> -<!-- we'll need this when we combine add/create for storage
> <div id="toolbar_nav">
> <ul>
> - <li class="current" id="unassigned_tab"><a href="#" onClick = "show_unassigned()"><img style="vertical-align:middle;" src="" /> Unassigned Storage (<%= @unassigned %>)</a></li>
> - <li id="assigned_tab"><a href="#" onClick = "show_assigned()"><img style="vertical-align:middle;" src="" /> Assigned Storage (<%= @assigned %>)</a></li>
> + <li class="current" id="new_tab"><a href="#" onClick = "show_new()"><img style="vertical-align:middle;" src="" /> Add New Pool</a></li>
> + <li id="add_tab"><a href="#" onClick = "show_add()"><img style="vertical-align:middle;" src="" /> Import Existing Pool(s)</a></li>
> </ul>
> </div>
> - function show_unassigned()
> +
> +<div class="panel_header"></div>
> +<div id="add_panel">
> + <div class="dialog_body_small">
> + <%= render :partial => "/storage/grid", :locals => { :table_id => "addstorage_grid",
> + :hwpool => nil, :exclude_id => @hardware_pool.id,
> + :on_select => "false" } %>
> + </div>
> + <%= popup_footer("add_storage('#{url_for :controller => 'hardware',
> + :action => 'add_storage',
> + :id => @hardware_pool}')",
> + "Add Storage Pool(s)") %>
> +</div>
> +
> +<div id="new_panel">
> + <%= render :partial => "/storage/new",
> + :locals => { :hardware_pool => @hardware_pool,
> + :storage_types => @storage_types} %>
> +</div>
> +
> +<script type="text/javascript">
> + function show_new()
> {
> - $("#assigned_tab").removeClass('current');
> - $("#unassigned_tab").addClass('current');
> - $("#addstorage_assigned_grid_div").hide()
> - $("#addstorage_unassigned_grid_div").show()
> + $("#add_tab").removeClass('current');
> + $("#new_tab").addClass('current');
> + $("#add_panel").hide()
> + $("#new_panel").show()
> +
> }
> - function show_assigned()
> + function show_add()
> {
> - $("#assigned_tab").addClass('current');
> - $("#unassigned_tab").removeClass('current');
> - $("#addstorage_unassigned_grid_div").hide()
> - $("#addstorage_assigned_grid_div").show()
> + $("#new_tab").removeClass('current');
> + $("#add_tab").addClass('current');
> + $("#new_panel").hide()
> + $("#add_panel").show()
> }
> - $("#addstorage_assigned_grid_div").hide()
> --->
> - <div class="panel_header"></div>
> - <div class="dialog_body">
> - <%= render :partial => "/storage/grid", :locals => { :table_id => "addstorage_grid",
> - :hwpool => nil, :exclude_id => @hardware_pool.id,
> - :on_select => "false" } %>
> - </div>
> -</div>
> -<%= popup_footer("add_storage('#{url_for :controller => 'hardware',
> - :action => 'add_storage',
> - :id => @hardware_pool}')",
> - "Add Selected Storage Pools") %>
> -
> + $("#add_panel").hide()
> +</script>
> diff --git a/wui/src/public/javascripts/ovirt.js b/wui/src/public/javascripts/ovirt.js
> index 8c94887..30262b5 100644
> --- a/wui/src/public/javascripts/ovirt.js
> +++ b/wui/src/public/javascripts/ovirt.js
> @@ -174,4 +174,31 @@ function empty_summary(element_id, label){
> }
>
>
> + function get_selected_storage()
> + {
> + return get_selected_checkboxes("storage_grid_form")
> + }
> + function validate_storage_for_move()
> + {
> + if (validate_selected(get_selected_storage(), 'storage pool')) {
> + $('#move_link_hidden').click()
> + }
> + }
> + function validate_storage_for_remove()
> + {
> + if (validate_selected(get_selected_storage(), 'storage pool')) {
> + $('#remove_link_hidden').click()
> + }
> + }
> + function delete_or_remove_storage()
> + {
> + var selected = $('#remove_storage_selection :radio:checked')
> + if (selected[0].value == "remove") {
> + remove_storage()
> + } else if (selected[0].value == "delete") {
> + delete_storage()
> + }
> + jQuery(document).trigger('close.facebox')
> + }
> +
>
> diff --git a/wui/src/public/stylesheets/components.css b/wui/src/public/stylesheets/components.css
> index e369330..56cadf2 100644
> --- a/wui/src/public/stylesheets/components.css
> +++ b/wui/src/public/stylesheets/components.css
> @@ -85,6 +85,9 @@
> #toolbar_nav li:hover {
> background: #4B95B8;
> }
> +#toolbar_nav li.current {
> + background: #4B95B8;
> +}
> #toolbar_nav li, #toolbar_nav li a {
> text-decoration: none;
> text-align: center;
Couple things on this. Generally, works fine for me, but when I click
on the new 'Import existing pool(s)' tab, it widens the popup so it
hangs off the right side of my screen. Looking at the markup, I see a
element style that has left:358px. Changing this in firebug moves the
box where it needs to go. I took a quick look at the facebox plugin,
and it looks like it build that container, so I think either we need to
call a function of facebox to recalculate its placement (which is
probably the right way), or calculate it ourselves and update the style
of the container element (guessing this would be more painful). The
place to do this manipulation would probably be in your show_add and
show_new js functions.
When I click on the above tab, the 'alias' field is also not fully
visible, and I cannot expand the column to read it (this may have been
an existing bug I never noticed).
Lastly, one nit. While technically you are not required to end
javascript lines with a semicolon, in my experience it is generally
considered best practice to do so. I know it might take a little
thought hopping back and forth between this and ruby's lack of
semicolons, but if there are no major objections, I would like to see us
standardize our code to always use them (so would require a few
additional changes to this patch).
More information about the ovirt-devel
mailing list