-
-
Notifications
You must be signed in to change notification settings - Fork 569
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
Use modernized waitMe plugin #3213
base: development
Are you sure you want to change the base?
Conversation
There is no |
<script src="<?=pihole.fileversion('vendor/waitMe/waitMe.min.js')?>"></script> | ||
<link rel="stylesheet" href="<?=pihole.fileversion('vendor/waitMe/waitMe.min.css')?>"> | ||
<script src="<?=pihole.fileversion('vendor/waitMe-js/modernized-waitme-min.js')?>"></script> | ||
<link rel="stylesheet" href="<?=pihole.fileversion('vendor/waitMe-js/waitMe.min.css')?>"> |
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.
Any reason this CSS is here? ideally it should be placed along with the rest vendor CSS files
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.
Have a look at the folder structure. We don't separate by Js and css anymore, but created a folder for each dependencies containing all their necessary stuff. This made it way easier to keep things up-to-date
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.
The point here is that CSS is better of grouped together because it helps the browser to plan ahead.
By mixing JS and CSS files we are certainly not helping. I would keep the CSS file along with the rest.
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
1 similar comment
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Nice to see the jQuery usage to drop even if it's a little :) That being said, I'd move this package to the vendor folder for now and move on if it works OK like the previous plugin. |
Signed-off-by: yubiuser <[email protected]>
Conflicts have been resolved. |
What does this PR aim to accomplish?:
Updates the used
waitMe
package because it's outdated and depends on an outdatedjquery
versionHow does this PR accomplish the above?:
Replace the package with a modernized version found at https://github.com/carlosvidal/modernized-waitme which does not rely on
jquery
By submitting this pull request, I confirm the following:
git rebase
)