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

Refactor repo-legacy.js, remove messy global variables. Fix errors. #17646

Merged
merged 15 commits into from Nov 18, 2021
Merged

Refactor repo-legacy.js, remove messy global variables. Fix errors. #17646

merged 15 commits into from Nov 18, 2021

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Nov 15, 2021 •
edited

The JS global variable commentMDEditors and autoSimpleMDE are removed. JS code uses $('textarea').data('simplemde') directly.

Two JS errors are also fixed:

  • Fix an error in Sortable
  • Fix an incorrect call assignMenuAttributes from the template

And the nodejs version mentioned in document is too old, so I just changed it to Node.js LTS :)

@wxiaoguang
Refactor repo-legacy.js, remove messy global variables. Fix errors.
4ac9069 
Fix an error in Sortable
Fix a incorrect call assignMenuAttributes from the template
@wxiaoguang
Merge branch 'main' into refactor-legacy-js
65318fd
@wxiaoguang wxiaoguang added type/refactoring Existing code has been cleaned up. There should be no new functionality. topic/ui Change the appearance of the Gitea UI skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Nov 15, 2021
@wxiaoguang wxiaoguang added this to the 1.16.0 milestone Nov 15, 2021
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 15, 2021
@silverwind
Copy link
Member

Good refactor, these globals were a very ugly hack.

wxiaoguang and others added 6 commits November 16, 2021 09:45
@wxiaoguang @silverwind
Update web_src/js/features/repo-legacy.js
22b3475 
Co-authored-by: silverwind <me@silverwind.io>
@wxiaoguang @silverwind
Update web_src/js/features/repo-legacy.js
5f4a6c0 
Co-authored-by: silverwind <me@silverwind.io>
@wxiaoguang @silverwind
Update web_src/js/features/repo-legacy.js
056f103 
Co-authored-by: silverwind <me@silverwind.io>
@wxiaoguang @silverwind
Update web_src/js/features/repo-legacy.js
e4f6a3f 
Co-authored-by: silverwind <me@silverwind.io>
@wxiaoguang @silverwind
Update web_src/js/features/repo-legacy.js
f7a960b 
Co-authored-by: silverwind <me@silverwind.io>
@wxiaoguang
Merge branch 'main' into refactor-legacy-js
ce7b9eb
@wxiaoguang
Copy link
Contributor Author

The legacy code is still very difficult to understand, so we have to continue to refactor them again and again.

ps: during refactoring, usually I just do copy and paste to untouched code and keep most code as before.

@silverwind
Copy link
Member

silverwind commented Nov 16, 2021 •
edited

I guess this editor stuff should ideally be rewritten as a Vue (or dare I say React) component. Things like 2-way binding in imperative jQuery-style code will always a pain.

@wxiaoguang
Copy link
Contributor Author

I agree to introduce some MVVM mechanism. Since we are still using these jQuery code heavliy, I think it's good to make them clear first. And if the legacy code becomes clear, it's easier to migrate them to new frameworks. That's why I am doing some refactoring work.

Gusted
Gusted approved these changes Nov 18, 2021
View reviewed changes
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

LGTM

@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 Nov 18, 2021
@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 Nov 18, 2021
@silverwind
Copy link
Member

Code looks good but I haven't gotten around to testing this.

@wxiaoguang
Copy link
Contributor Author

Code looks good but I haven't gotten around to testing this.

I have tested it on my side. If there is any bug, I will fix it ASAP.

@wxiaoguang wxiaoguang merged commit 55be5fe into go-gitea:main Nov 18, 2021
@wxiaoguang wxiaoguang deleted the refactor-legacy-js branch November 18, 2021 16:45
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@wxiaoguang
Refactor repo-legacy.js, remove messy global variables. Fix errors. (g…
831e084 
…o-gitea#17646)

Refactor repo-legacy.js, remove messy global variables. Fix errors.
Fix an error in Sortable
Fix a incorrect call assignMenuAttributes from the template
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny approved these changes

@Gusted Gusted Gusted approved these changes

@silverwind silverwind Awaiting requested review from silverwind

@lafriks lafriks Awaiting requested review from lafriks

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/ui Change the appearance of the Gitea UI type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.16.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@wxiaoguang @silverwind @lunny @Gusted @GiteaBot