Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove jQuery AJAX from the repo-issue.js file #29776

Merged
merged 13 commits into from
Mar 14, 2024
Merged

Remove jQuery AJAX from the repo-issue.js file #29776

merged 13 commits into from
Mar 14, 2024

Conversation

yardenshoham
Copy link
Member

Removed all jQuery AJAX calls and replaced with our fetch wrapper.

Tested the following functionalities and they work as before:

  • due-date update
  • comment deletion
  • branch update by merge or rebase
  • allow edits from maintainers button
  • reviewer addition or deletion
  • WIP toggle button
  • new diff code comment button
  • issue title edit button

Demo using fetch instead of jQuery AJAX

Updating the due-date of an issue

due_date

Deleting a comment

comment_delete

Updating a branch in a pull request

branch_update

Checking and unchecking the "Allow edits from maintainers" checkbox

allow_edits

Requesting review and removing review request

reviewer_addition

Toggling the WIP status of a pull request

wip

Clicking the new code comment button on the diff page

code_comment

Editing the issue title and target branch

issue_title

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 13, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 13, 2024
@yardenshoham yardenshoham added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 13, 2024
silverwind
silverwind reviewed Mar 13, 2024
View reviewed changes
web_src/js/features/repo-issue.js
}
let data;
try {
data = await response?.json(); // the response is probably not a JSON
Copy link
Member

@silverwind silverwind Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move line into above try imho and remove the ?.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response isn't a JSON though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we find out what responses are possible?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It returns the full page (HTML)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always? Then why do we try to parse JSON? :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which demons did we summon now?
And why does this sound like HTMX to me?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it but the URL is dynamic and we don't replace content, we reload. It's certainly possible but I'd have to add some form and inputs and have the HX-Refresh header in the response so I decided it's not worth it

@yardenshoham @silverwind
Update web_src/js/features/repo-issue.js
c49a87b 
Co-authored-by: silverwind <me@silverwind.io>
@yardenshoham @silverwind
Update web_src/js/features/repo-issue.js
72d14d7 
Co-authored-by: silverwind <me@silverwind.io>
delvh
delvh reviewed Mar 13, 2024
View reviewed changes
web_src/js/features/repo-issue.js
}
let data;
try {
data = await response?.json(); // the response is probably not a JSON
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which demons did we summon now?
And why does this sound like HTMX to me?

web_src/js/features/repo-issue.js Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 13, 2024
silverwind
silverwind approved these changes Mar 14, 2024
View reviewed changes
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok if it works

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 14, 2024
@silverwind silverwind enabled auto-merge (squash) March 14, 2024 21:32
@yardenshoham yardenshoham added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 14, 2024
@silverwind silverwind removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 14, 2024
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 14, 2024
@silverwind silverwind enabled auto-merge (squash) March 14, 2024 21:33
@silverwind silverwind merged commit 0679e60 into go-gitea:main Mar 14, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Mar 14, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 14, 2024
@yardenshoham yardenshoham deleted the repo-issue-jquery-ajax branch March 14, 2024 21:37
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 15, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
9859580 
* giteaofficial/main:
  Update JS dependences (go-gitea#29797)
  Unify search boxes (go-gitea#29530)
  Fix document error about 'make trans-copy' (go-gitea#29710)
  Remove jQuery AJAX from the diff functions (go-gitea#29743)
  Fix Safari spinner rendering (go-gitea#29801)
  Remove jQuery AJAX from the `repo-issue.js` file (go-gitea#29776)
  Improve commit record's ui in comment list (go-gitea#26619)
  enable tailwind nesting (go-gitea#29746)
@6543 6543 modified the milestones: 1.23.0, 1.22.0 Mar 16, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jun 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@delvh delvh delvh approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/js size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
6 participants
@yardenshoham @silverwind @wxiaoguang @delvh @GiteaBot @6543