-
Notifications
You must be signed in to change notification settings - Fork 993
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 #21641 - Drop jquery-ui spinners in favor of React implementation #8538
Conversation
Issues: #21641 |
The failures are definitelly not relevant, probably some npm package update? |
This shouldn't be merged before it's usage is removed in plugins: |
there is multiple levels to answer this.
This being sad, we could leave blank function with the deprecation warning only for one more version, so the plugins don't get js errors just by calling useless function. |
If it's a simple migration, it can be beneficial (and faster) to submit a PR yourself. |
@@ -46,7 +45,6 @@ window.tfm = Object.assign(window.tfm || {}, { | |||
hosts, | |||
httpProxies, | |||
toastNotifications, | |||
numFields, |
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 can imagine having in this file
const numFieldsDeprecationOnly = {
initAll: () => { window.tfm.tools.deprecate('initAll()', 'does nothing as of now, please stop calling it', '3.2') };
};
And then here: numFields: numFieldsDeprecationOnly,
Which would prevent plugins from failing hard on the function missing for two more version.
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.
Nice work!
Just noticed we are leaving some bits behind, can we drop these requires? :)
//= require jquery-ui/widgets/spinner foreman/app/assets/stylesheets/application.scss
Lines 5 to 6 in dcf1135
@import 'jquery-ui/spinner'; @import 'jquery.ui.spinner_custom'; - [whole file] https://github.com/theforeman/foreman/blob/dcf11351d798bc59886fded3e0dfbff01ea98482/app/assets/stylesheets/jquery.ui.spinner_custom.scss
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.
Thanks @yifatmakias ! Great drop 👍
No description provided.