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

Fixes #37382 - Update to jQuery 3 #10138

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

@MariaAga MariaAga requested a review from a team as a code owner April 24, 2024 12:46
@github-actions github-actions bot added UI Legacy JS PRs making changes in the legacy Javascript stack labels Apr 24, 2024
@ekohl
Copy link
Member

ekohl commented Apr 24, 2024

I don't see a bump of jquery itself. Is this removing deprecated jquery < 3 usages?

@MariaAga
Copy link
Member Author

@ekohl jquery is in theforeman/foreman-js#479

Comment on lines 172 to 233
$('#check_all_roles').click(function(e) {
$('#check_all_roles').on('click', function(e) {
e.preventDefault();
$('.role_checkbox').prop('checked', true);
});

$('#uncheck_all_roles').click(function(e) {
$('#uncheck_all_roles').on('click', function(e) {
Copy link
Member

Choose a reason for hiding this comment

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

I think these are both unused since acfbc45. There the links with check_all_roles and uncheck_all_roles IDs were removed. They were originally added in feacea3.

I've submitted #10141 to drop them.

I also noticed toggleRowGroup, filter_permissions and typeToIcon are now unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

rebased to include that pr

@@ -1,11 +1,12 @@
group :assets do
gem 'jquery-ui-rails', '~> 6.0'
gem 'jquery-ui-rails', '~> 7.0'
Copy link
Member

Choose a reason for hiding this comment

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

Do we still use this? I don't see anything using this. I get the impression that since b55d3ed it's unused because we no longer have require jquery-ui in the code, but we do still have @import jquery-ui. So we load the CSS, but not any of the actual JS code?

Can we drop it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so #10142

$.fn.bind = function(event, func) {
return this.on(event, func);
};
// used by puppet plugin
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps open a PR against foreman_puppet to drop this usage?

Copy link
Member Author

Choose a reason for hiding this comment

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

Took a while to setup puppet, but made the pr: theforeman/foreman_puppet#403

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

What's your plan for this? Could we already start using JSON.parse (which AFAIK is native browser functionality)?

Looks like that is also something that needs to be done in multiple repositories. From quickly grepping I find:
https://github.com/theforeman/foreman_puppet/blob/6aa2761cc37bec424c05265a72043706162e3b02/webpack/src/foreman_class_edit.js#L157
https://github.com/theforeman/foreman_puppet/blob/6aa2761cc37bec424c05265a72043706162e3b02/webpack/src/foreman_class_edit.js#L185
https://github.com/theforeman/foreman_openscap/blob/99c00ff3a4c948f660ebe81180cc780c8ff5b0d8/app/assets/javascripts/foreman_openscap/arf_reports.js#L5
https://github.com/Katello/katello/blob/b94f81d09510d135dcc1bdce8a8ef8c2a48c6f39/app/views/katello/sync_management/index.html.erb#L12
https://github.com/Katello/katello/blob/b94f81d09510d135dcc1bdce8a8ef8c2a48c6f39/app/assets/javascripts/katello/common/katello.js#L34

Then another thing we can already do is move to js-cookie. That's something that happens in various repositories. AFAIK jquery's cookie implementation and js-cookie don't conflict, so we can add js-cookie and then migrate various plugins.

@MariaAga
Copy link
Member Author

MariaAga commented Jul 18, 2024

Good idea, I'll move it here: #10250 and will create some prs in the plugins

}
}
$(this).select2({
debug: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

Should remove the debug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Legacy JS PRs making changes in the legacy Javascript stack Needs re-review UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants