-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Remove jQuery AJAX from the archive download links #29380
Remove jQuery AJAX from the archive download links #29380
Conversation
- Removed all jQuery AJAX calls and replaced with our fetch wrapper - Tested the repo collaborator mode dropdown functionality and it works as before Signed-off-by: Yarden Shoham <git@yardenshoham.com>
BTW I'm not sure that logic adding the |
web_src/js/features/repo-common.js
Outdated
const data = await response.json(); | ||
if (!data) { | ||
// XXX Shouldn't happen? | ||
$target.closest('.dropdown').children('i').removeClass('loading'); |
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.
put the loading removal in a finally
clause . This is important because fetch will throw on network errors.
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.
It shouldn't happen always so I put it in a catch
but anyway these loading indicators are not working
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.
Now you have 3 code branches that remove the indicator, while a single finally
would be sufficient as long as you don't return
inside the try
. Actually I think a finally
may even execute when you return
.
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.
Now you have 3 code branches that remove the indicator, while a single
finally
would be sufficient as long as you don'treturn
inside thetry
. Actually I think afinally
may even execute when youreturn
.
Yes, finally
executes always, even after return
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.
In fact it does:
(() => {
try {
return console.log('a');
} finally {
console.log('b');
}
})();
Logs both "a" and "b".
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.
In the case complete
is false we don't want to remove the loading indicator
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.
How about setting a let isComplete = true
variable before the fetch, set it to false
conditionally and check it during indicator removal and before the recursion?
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 code as it is right now has the same behavior as before. Logic changes should not go in this PR
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 code reads like ass but previous one did too, so whatever.
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.
Agreed on that front
* giteaofficial/main: (45 commits) Include resource state events in Gitlab downloads (go-gitea#29382) Add API to get PR by base/head (go-gitea#29242) [skip ci] Updated translations via Crowdin Improve Documentation for Restoration from backup (go-gitea#29321) Refactor "user/active" related logic (go-gitea#29390) Remove jQuery AJAX from the archive download links (go-gitea#29380) Add tailwindcss (go-gitea#29357) Add missing space (go-gitea#29393) Integrate alpine `noarch` packages into other architectures index (go-gitea#29137) enforce maxlength in frontend (go-gitea#29389) Remove incorrect and unnecessary Escape from templates (go-gitea#29394) Make actions animation rotate counterclockwisely (go-gitea#29378) Use `crypto/sha256` (go-gitea#29386) Add `io.Closer` guidelines (go-gitea#29387) Remove jQuery AJAX from the notice selection deletion button (go-gitea#29381) Refactor Safe modifier (go-gitea#29392) Add attachment support for code review comments (go-gitea#29220) Refactor modules/git global variables (go-gitea#29376) Remove jQuery from the code diff expansion buttons (go-gitea#29385) Remove jQuery AJAX from the markdown editor preview (go-gitea#29384) ...
Demo using
fetch
instead of jQuery AJAX