-
Notifications
You must be signed in to change notification settings - Fork 74
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
Remove switch_helper and remove obsolete templates #4024
base: update_jquery_switch_js
Are you sure you want to change the base?
Remove switch_helper and remove obsolete templates #4024
Conversation
2f22029
to
1e1ab87
Compare
e7d6d40
to
3215c96
Compare
What's the relation between this and #4017? Aren't they incompatible? |
Well, this was a suggestion for #4017 branch that was partially applied by @josemigallas Now I could probably rebase it so some extra suggestions are compatible 😬 |
1e1ab87
to
a6c9208
Compare
a6c9208
to
6f702e7
Compare
$("#referrer-filters-form").hide() | ||
$("#referrer-filters-limit").show() |
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.
We could keep .enabled_block
/ .disabled_block
here, but I thought more explicit names were better.
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.
This works fine for me. If the package is indeed not needed, I vote for removing.
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.
Was this not being used?
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.
Same, Was this not being used?
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.
Yeah, in the PR description I mentioned why I think this file is here. I can't see it being used anywhere, and I think it was not used in the older portals either.
And I think _help
has never been used either (for the dev portal).
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.
in the PR description I mentioned why I think this file is here
Ah right, sorry 😥
Removing
switch_helper
because we really can live without it.Regarding
lib/developer_portal/app/views/developer_portal/admin/applications/referrer_filters/_widget.html.erb
andlib/developer_portal/app/views/developer_portal/admin/applications/referrer_filters/_help.html.erb
removal, it was quite tricky to follow the history of the developer portal's Application Show template, because it moved so many times within the repository, but I am pretty confident that it actually never relied on thewidget
partial, and the presense of it is just a result of copy-paste the templates when the ancients splitted the admin and developer portal templates.I believe that this template has used the following content since the beginning of time:
https://github.com/3scale/system/pull/2536/files#diff-3c9dd32a92e0229e1d80def94ead471afb71a3b0896880e438587a2dd1962381
(look for
vendor/developer_portal/.default-templates/applications/show.html.liquid
)It uses this block with explicit class names:
and is not using
referrer_filters/widget
unlike the provider side, and so it was not using theswitch_helper
either (probably it can't because its Liquid).