-
Notifications
You must be signed in to change notification settings - Fork 27
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
Failed upload job limiter #386
base: master
Are you sure you want to change the base?
Conversation
There is currently a failure in upstream of |
It works! |
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.
Thank you, this looks great to me! Apart from the notification stuff, I only have one wish/suggestion: Moodle strongly discourages the use of jQuery in new Code, so if you have the time, it would be great to use native JS instead of jQuery in new Code. But we'll have to go through the javascript code anyway someday and replace all the jQuery, so I would also merge it if you don't do that.
Hi @justusdieckmann, thanks for your review. About the JS and JQuery, I would say, because it is a part of index js file, therefore, it gets a bit messy if I change it to js now, and that would be great, if we could take care of it later on in one single go! Regarding this, I provided new es6 js for my new PR but in here, it is not feasible! BR |
Hi @justusdieckmann; |
This PR fixes #385,
What is new