-
Notifications
You must be signed in to change notification settings - Fork 437
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
Move partials to its own directory. #5869
Conversation
@@ -0,0 +1,5 @@ | |||
module Webui::Packages::PartialsPathHelper | |||
def partial_path(directory, partial) | |||
File.join('webui2', 'webui', 'package', directory, partial) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to have code for this. It's already possible to achieve this by specifying the full path to the partial starting from app/views
. This is what this code does too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking with @vpereira, I am also fine with this approach. I prefer not having an helper, but it's not a issue to have it too. I won't block this PR due to this.
@@ -0,0 +1,5 @@ | |||
module Webui::Packages::PartialsPathHelper | |||
def partial_path(directory, partial) | |||
File.join('webui2', 'webui', 'package', directory, partial) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking with @vpereira, I am also fine with this approach. I prefer not having an helper, but it's not a issue to have it too. I won't block this PR due to this.
%ul.list-unstyled | ||
- if failures > 0 | ||
= render partial: 'show_failures', locals: { failures: failures, package_name: package.name, project: project } | ||
= render partial: partial_path('side_links', 'show_failures'), locals: { failures: failures, package_name: package.name, project: project } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do take issue with this. I know what render partial: 'directory/subdirectory/file'
does, I have to look up the meaning and implementation of partial_path
. It is code we do not need, which is the worst kind of code. Sorry...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR
my bikeshed is green 👽
actually, as soon as we get rid of webui/webui2
is easier to change in one place than switch it everywhere, we could put it in a variable, but that's exactly why helpers are there @hennevogel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But once we switched to webui2 we would drop the partial_path
or not? And is this planned to be used only for package partials or others as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bikeshed is green
If I would think this is not important I would mark my comment as nitpick. I haven't 🤠
2a2f05c
to
7656dc4
Compare
7656dc4
to
7932395
Compare
7932395
to
eafbb5b
Compare
In the #5854 I refactored the _sidelinks view, however I think that all partials should be packed together in it's own directory (to avoid clutter), named sidelinks