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 .text() #30506

Merged
merged 23 commits into from
Jun 10, 2024
Merged

Remove jQuery .text() #30506

merged 23 commits into from
Jun 10, 2024

Conversation

silverwind
Copy link
Member

Remove and forbid .text(). Tested some, but not all functionality, but I think these are pretty safe replacements.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 15, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 15, 2024
@silverwind silverwind force-pushed the notext branch 2 times, most recently from 77ecf85 to 14bd166 Compare April 15, 2024 20:21
@silverwind silverwind added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Apr 15, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 16, 2024 •
edited
Loading

As I said in Propose to restart 1.22 release #30501 , I have objection for merging more pure refactoring (non-bug-related) PRs into main branch in recent weeks.

We should focus on making 1.22 release stable and fix regression bugs at the moment.

Just wait for some time to make the main branch stable enough, release 1.22 and start the refactoring when we do not need to heavily fix regression bugs.

delvh
delvh approved these changes Apr 16, 2024
View reviewed changes
web_src/js/features/notification.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 Apr 16, 2024
wxiaoguang
wxiaoguang previously requested changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Wait for 1.22 to be a stable release

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 16, 2024
@silverwind
Copy link
Member Author

Wait for 1.22 to be a stable release

This PR will only merge into main, it will not be backported. What is the issue?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 16, 2024 •
edited
Loading

Wait for 1.22 to be a stable release

This PR will only merge into main, it will not be backported. What is the issue?

The proposal is described in #30501

image

Refactoring without full test would cause many regressions during the 1.22 release period. I believe Gitea shouldn't introduce too many regressions in a RC version.

I think we can take a look at the recent fixed regression bugs by such blindly refactoring, there are a lot, and there are still unfixed ones.

Details about recent fixes & regressions:

(and there are more .....)

There are enough regressions IMO

@wxiaoguang
Copy link
Contributor

To be clear, I never meant that I don't like refactoring, instead, I always prefer to refactor legacy code to make it more maintainable. I don't care about controllable regressions during development stage, sometimes I also introduce regressions.

I only mean that during this release period, we need to focus on making the release as stable as possible.

@silverwind
Copy link
Member Author

I still don't understand why you block this. We fork the release branch, give it 1-2 months of backports and that's generally stable enough for a release. That way, we never have to delay refactors and that has worked well in the past. No idea why you suddenly want to change this process.

@wxiaoguang
Copy link
Contributor

I still don't understand why you block this. We fork the release branch, give it 1-2 months of backports and that's generally stable enough for a release. That way, we never have to delay refactors and that has worked well in the past. No idea why you suddenly want to change this process.

I think I didn't "suddenly" change this process.

Actually, I am trying to make Gitea's release to following the history procedure. I started writing PRs since 1.15 or 1.16, from that time, I seldom saw big refactoring during the RC period. Only some are done for various necessary purposes.

But I don't understand why 1.22 and 1.23 it should start the refactoring too urgent, it would make more regressions and make backport quite difficult.

In old release, I never saw the backport became difficult.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Apr 16, 2024
@wxiaoguang
Copy link
Contributor

I dismiss my change request.

But I keep my opinion: do the best to make Gitea stable during the release, and make bugfixes easy to backport.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 16, 2024 •
edited
Loading

ps: there is another open regression: on the release page, "download source code archive" causes JS error and doesn't work, I think it should also be fixed.

-> Refactor and fix archive link bug #30535

@wxiaoguang wxiaoguang mentioned this pull request Apr 17, 2024
Merged
@wxiaoguang
Copy link
Contributor

Another sample, 1.22 is broken for backporting: #30570 (comment)

@silverwind
Copy link
Member Author

Another sample, 1.22 is broken for backporting: #30570 (comment)

That's just a natural side-effect of changes. Nothing we can do about this except releasing more frequently.

@silverwind
Copy link
Member Author

Tested almost everything now and did ancillary jquery removal while testing.

showDeletePopup is broken and work in progress.

@silverwind
Copy link
Member Author

Okay, delete-button is fixed and meticulously tested now. It uses .dataset because subsequent code in frontend and backend depends on this camel-casing.

There is one notable difference between that $().data and .dataset, where the former does this type convertion but ultimately it does not matter for the functionionality of this POST request because number and string are represented exactly the same in a FormData body.

@silverwind silverwind marked this pull request as ready for review June 6, 2024 23:06
@silverwind
Copy link
Member Author

Ready again, now everything is tested and it was good to do so because there were bugs in the previous implementation.

@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Jun 10, 2024
@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 Jun 10, 2024
@lunny lunny merged commit a2304cb into go-gitea:main Jun 10, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Jun 10, 2024
@silverwind silverwind deleted the notext branch June 10, 2024 10:48
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 11, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
17f77f3 
* giteaofficial/main:
  Fix line number width in code preview (go-gitea#31307)
  Delete legacy cookie before setting new cookie (go-gitea#31306)
  [skip ci] Updated translations via Crowdin
  Use `querySelector` over alternative DOM methods (go-gitea#31280)
  Remove jQuery `.text()` (go-gitea#30506)
  [skip ci] Updated translations via Crowdin
  Remove sub-path from container registry realm (go-gitea#31293)
  Fix some URLs whose sub-path is missing (go-gitea#31289)
@kemzeb kemzeb mentioned this pull request Jun 15, 2024
Closed
@wxiaoguang wxiaoguang mentioned this pull request Jun 15, 2024
Merged
@wxiaoguang
Copy link
Contributor

-> Fix JS error when creating new issue #31383

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh delvh approved these changes

@wxiaoguang wxiaoguang wxiaoguang 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/internal modifies/js modifies/templates This PR modifies the template files 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.23.0
Development

Successfully merging this pull request may close these issues.
5 participants
@silverwind @wxiaoguang @delvh @lunny @GiteaBot