Fixes #26181 - move settings.js to webpack #6517
Conversation
|
Issues: #26181 |
|
thanks @amirfefer every change that remove a full page reload between page loads is a great improvement! |
| @@ -8,7 +8,6 @@ | |||
| //= require vendor | |||
| //= require jquery.extentions | |||
| //= require jquery.multi-select | |||
| //= require settings | |||
There was a problem hiding this comment.
actually, re-reading this, it looks like settings was always in the application bundle, hence it didn't really trigger a full page reload....
|
|
||
| $(document).ready(() => { | ||
| if (window.location.pathname === window.foreman_url('/settings')) { | ||
| $('.editable').editable({ |
There was a problem hiding this comment.
This is probably my lack of knowledge, but how does this get the editable since it's a plugin? Do you need to import the plugin or does it rely on some side effect somewhere?
There was a problem hiding this comment.
interesting... we require it over the assets pipeline
//= require editable/bootstrap-editable
//= require editable/railsin application.js, and we load application.js after webpack files, therefore it works
There was a problem hiding this comment.
That sounds rather fragile to rely on the classic pipeline in the webpack pipeline. AFAIK the settings are all defined in Foreman models with the actual values being added by plugins. That means the page is probably not modified by plugins. Wouldn't it be better to rewrite it to react rather than to risk breaking it by moving things if there's no actual gain right now?
There was a problem hiding this comment.
Also, are we sure editable isn't used by any other code? Any plugins using the x-editable-rails gem?
| @@ -0,0 +1,14 @@ | |||
| import $ from 'jquery'; | |||
|
|
|||
| $(document).ready(() => { | |||
|
|
||
| $(document).ready(() => { | ||
| if (window.location.pathname === window.foreman_url('/settings')) { | ||
| $('.editable').editable({ |
There was a problem hiding this comment.
this will register the callback each time you go to a screen like that, is there a chance it will create multiple on click events or similar?
|
@amirfefer whats the status of this PR? thanks |
|
can you rebase ? :) |
|
@amirfefer do you intend to continue working on this or shall we close it? |
|
@tbrisker - I'm focusing on other things atm, closing this for now. |
No description provided.