Refactor bottom_actions Package view: Break it in small partials #5867
Conversation
vpereira
commented
Sep 17, 2018
vpereira
commented
- bottom_actions/bugzilla_owner
- bottom_actions/branch_package
- bottom_actions/submit_package
- bottom_actions/edit_description
- bottom_actions/delete_package
- bottom_actions/cloud_upload
- bottom_actions/trigger_services
- bottom_actions/view_kiwi
- bottom_actions/request_deletion
- bottom_action/request_devel_project_change
- bottom_action/request_role_adiction
vpereira
force-pushed
the
refactor_bottom_actions
branch
from
57018c0
to
37841f2
Compare
vpereira
force-pushed
the
refactor_bottom_actions
branch
from
37841f2
to
9c477fd
Compare
| Delete package | ||
| = render partial: 'webui2/webui/package/bottom_actions/edit_description', locals: { project: project, package: package, spec_count: spec_count } | ||
| = render partial: 'webui2/webui/package/bottom_actions/delete_package' | ||
|
|
||
| - Feature.with(:kiwi_image_editor) do |
There was a problem hiding this comment.
I don't see the point of having these conditions out of the partial
There was a problem hiding this comment.
to be able to understand the workflow of a view? i.e you are just rendering the views A,B if current_user can modify it, just render view C,D if current_rev isn't nil. So you don't have to open all partials to understand it. You can just open the "main partial" to understand it. All the others are dumb, no logic.
There was a problem hiding this comment.
I agree with @vpereira. The partials are also not rendered if not needed.
There was a problem hiding this comment.
I do not entirely agree, but I don't want block this PR.
| %i.fas.fa-compact-disc.text-primary | ||
| View Image | ||
| = render partial: 'webui2/webui/package/bottom_actions/view_kiwi', locals: { package_id: package.id } | ||
|
|
||
| - if Feature.active?(:cloud_upload) && !User.current.is_nobody? |
There was a problem hiding this comment.
&& !User.current.is_nobody? is not needed here because it's already checked above in unless User.current.is_nobody?
| Delete package | ||
| = render partial: 'webui2/webui/package/bottom_actions/edit_description', locals: { project: project, package: package, spec_count: spec_count } | ||
| = render partial: 'webui2/webui/package/bottom_actions/delete_package' | ||
|
|
||
| - Feature.with(:kiwi_image_editor) do |
There was a problem hiding this comment.
I agree with @vpereira. The partials are also not rendered if not needed.
- bottom_actions/bugzilla_owner - bottom_actions/branch_package - bottom_actions/submit_package - bottom_actions/edit_description - bottom_actions/delete_package - bottom_actions/cloud_upload - bottom_actions/trigger_services - bottom_actions/view_kiwi - bottom_actions/request_deletion - bottom_action/request_devel_project_change - bottom_action/request_role_adiction
vpereira
force-pushed
the
refactor_bottom_actions
branch
from
9c477fd
to
a7f7e49
Compare
|
I would consider clarifying more the execution flow by grouping the buttons in "high" level partials. That way you will have something like this: %ul.list-inline.mb-0
- if bugowners_mail.present? && configuration['bugzilla_url']
= render partial: 'webui2/webui/package/bottom_actions/bugzilla_owner', locals: { bugowners_mail: bugowners_mail, package_name: package.name, project_name: project.name }
- unless User.current.is_nobody?
- if current_rev
= render partial: 'webui2/webui/package/bottom_actions/actual_revision', locals: { package: package, project: project, revision: revision }
- if User.current.can_modify?(package)
= render partial: 'webui2/webui/package/bottom_actions/user_can_modify', locals: { project: project, package: package, spec_count: spec_count, package_id: package.id, cloud_upload_index_path: cloud_upload_index_path }
- else
= render partial: 'webui2/webui/package/bottom_actions/user_cant_modify'
//TODO: only users w/o rights should see this, maintainers should get a different dialog:
- if package.develpackage
= render partial: 'webui2/webui/package/bottom_actions/request_devel_project_change', locals: { package: package, project: project } |
|
@vpereira thank you for your answer 🤔 |
