Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor package partial links (_side_links) #5854

Merged
merged 7 commits into from
Sep 14, 2018

Conversation

vpereira
Copy link
Contributor

Refactor the partial _side_links for packages, breaking it in smaller partials

@vpereira
Copy link
Contributor Author

= link_to "error#{failures == 1 ? '' : 's'}", project_monitor_path(project: project, pkgname: package.name, succeeded: 0,
blocked: 0, finished: 0, signing: 0, dispatching: 0,
scheduled: 0, building: 0)
= render partial: 'show_failures', locals: { failures: failures, pkgname: package.name, project: project }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nitpick]: Could you change pkgname to package_name, please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you are saying to change it in the view, doable nitpick, but I won't touch in the controller https://github.com/vpereira/open-build-service/blob/da08d94faa5c84c1ba3bc175c38c61584976cad2/src/api/app/controllers/webui/project_controller.rb#L390-L391 where the GET param name is pkgname

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vpereira, only in the view 👍

@dmarcoux
Copy link
Contributor

I would squash the commits, we don't need this amount of details when it's about splitting a partial. How about this Split partial package/_side_links.html.haml into smaller partials

@vpereira
Copy link
Contributor Author

I would squash the commits, we don't need this amount of details when it's about splitting a partial. How about this Split partial package/_side_links.html.haml into smaller partials

first I have to figure out why the tests aren't green. Therefore I prefer to have it now as single commits. Before merge I will squash it. thank you

@DavidKang DavidKang added Frontend Things related to the OBS RoR app Refactoring 🏭 labels Sep 14, 2018
@vpereira vpereira force-pushed the refactor_package_partial_links branch 3 times, most recently from 52bfe49 to 703e719 Compare September 14, 2018 12:59
@vpereira vpereira force-pushed the refactor_package_partial_links branch from 703e719 to bd6f525 Compare September 14, 2018 13:01
%li
%i.fas.fa-times-circle.text-danger{ title: 'Errors' }
= failures
= link_to "error#{failures == 1 ? '' : 's'}", project_monitor_path(project: project, pkgname: package_name, succeeded: 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use case for pluralize

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmarcoux, good catch

@vpereira vpereira merged commit f046371 into openSUSE:master Sep 14, 2018
@vpereira vpereira deleted the refactor_package_partial_links branch January 7, 2019 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Things related to the OBS RoR app Refactoring 🏭
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants