ACK with a minor note inline.
On 10/06/2010 12:01 PM, [email protected] wrote:
> From: martyntaylor<[email protected]>
>
> ---
> src/app/controllers/users_controller.rb | 17 +++++++++++++++++
> src/app/stylesheets/aggregator.scss | 20 +++++++++++++++++++-
> src/app/views/users/index.haml | 18 +++++-------------
> 3 files changed, 41 insertions(+), 14 deletions(-)
>
> diff --git a/src/app/controllers/users_controller.rb
> b/src/app/controllers/users_controller.rb
> index ca59382..61b97db 100644
> --- a/src/app/controllers/users_controller.rb
> +++ b/src/app/controllers/users_controller.rb
> @@ -104,6 +104,23 @@ class UsersController< ApplicationController
> end
> end
>
> + def manage_user
> + @current_user = current_user
> + type = params[:commit]
> + user_id = params[:user_checkbox]
> + if type&& User.exists?(user_id)
> + if type == "edit"
> + redirect_to edit_user_url(user_id)
> + elsif type == "delete"
> + params[:id] = user_id
> + destroy
> + end
> + else
> + flash[:notice] = "Error performing this operation"
> + redirect_to users_path
> + end
> + end
> +
> def destroy
> if @current_user.permissions.collect { |p| p.role }.find { |r| r.name
> == "Administrator" }
> if request.post? || request.delete?
> diff --git a/src/app/stylesheets/aggregator.scss
> b/src/app/stylesheets/aggregator.scss
> index c4300c4..4ece961 100644
> --- a/src/app/stylesheets/aggregator.scss
> +++ b/src/app/stylesheets/aggregator.scss
> @@ -124,6 +124,24 @@ input[type='submit'],button,.button {
> &.actionlink {
> margin: 40px 0 0;
> }
> +&.linkbutton {
> + background: none;
> + border:0;
> + margin:4px 4px 4px 5px;
> + color: $dcprimary;
> + text-decoration: none;
> + font:13px/1.5 "Liberation Sans","Droid Sans",Helvetica,Arial,sans-serif;
> +&:hover {
> + @include box-shadow(0, 0, 0, rgba(0,0,0,0));
> + text-decoration: underline;
> + color: lighten($dcprimary,20%);
> + }
> +&:active {
> + color: darken($dcprimary, 20%);
> + font:13px/1.5 "Liberation Sans","Droid
> Sans",Helvetica,Arial,sans-serif;
> + margin:4px 4px 4px 5px;
> + }
> + }
> &.disabled,&[disabled] {
> cursor: default;
> background: transparent;
> @@ -1113,4 +1131,4 @@ h3.first, h3.second, h3.third, h3.fourth, h3.fifth,
> h3.sixth, h3.seventh {
> width: 1020px;
> margin-left: -510px;
> background: url(/images/960.png) repeat-y top center;
> -}
> +}
> \ No newline at end of file
> diff --git a/src/app/views/users/index.haml b/src/app/views/users/index.haml
> index 0f88715..2c9ba20 100644
> --- a/src/app/views/users/index.haml
> +++ b/src/app/views/users/index.haml
> @@ -1,13 +1,14 @@
> += form_tag :action => 'manage_user'
> .grid_3.actionsidebar
> %dl
> %dt
> Users
> %dd.edit
> %span
> - = link_to "edit", edit_user_url(@users.first.id), :id => "edit_link"
> + = submit_tag "edit", :class => "submit linkbutton"
> %dd.delete
> %span
> - = link_to "delete", user_url(@users.first.id), :id =>
> "delete_link", :method => "delete"
> + = submit_tag "delete", :class => "submit linkbutton"
> %dd.create
> %span
> = link_to "create", new_user_url
> @@ -29,9 +30,9 @@
> %tr
> %td
> - if user == @users.first
> - %input{:checked => true, :name => "user_checkbox", :type =>
> "checkbox", :onchange => "update_link(#{user.id})", :id =>
> "user_checkbox_#{user.id}" }
> + %input{:checked => true, :name => "user_checkbox", :type =>
> "checkbox", :value => user.id, :onchange => "update_link(#{user.id})", :id
> => "user_checkbox_#{user.id}" }
> - else
> - %input{:checked => false, :name => "user_checkbox", :type =>
> "checkbox", :onchange => "update_link(#{user.id})", :id =>
> "user_checkbox_#{user.id}" }
> + %input{:checked => false, :name => "user_checkbox", :type =>
> "checkbox", :value => user.id, :onchange => "update_link(#{user.id})", :id
> => "user_checkbox_#{user.id}" }
The code inside the `if` and the `else` blocks is almost identical. I
think it would be better to avoid the repetition by removing the
condition entirely:
- is_first_user = (user == @users.first)
%input{:checked => is_first_user, :name => "user_checkbox", :type =>
"checkbox", :onchange => "update_link(#{user.id})", :id =>
"user_checkbox_ {user.id}" }
%input{:checked => is_first_user, :name => "user_checkbox", :type =>
"checkbox", :value => user.id, :onchange =>
"update_link(#{user.id})", :id => "user_checkbox_#{user.id}" }
It's not a big deal but it will help if we have to touch that code in
the future.
> %td= link_to user.login, user, :id => user.id
> %td= user.last_name
> %td= user.first_name
> @@ -45,17 +46,8 @@
> function update_link(id)
> {
> var checkbox = document.getElementById('user_checkbox_' + id)
> - var edit_link = document.getElementById('edit_link')
> - var delete_link = document.getElementById('delete_link')
> -
> - var edit_user_url = document.getElementById('edit_user_' + id)
> - var delete_user_url = document.getElementById('delete_user_' + id)
> -
> if(checkbox.checked)
> {
> - edit_link.href = edit_user_url.href
> - delete_link.href = delete_user_url.href
> -
> var checkboxes = document.getElementsByName("user_checkbox")
> for(var i = 0; i< checkboxes.length; i++)
> {
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
