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.

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

Refactor image diff #31444

Merged
merged 3 commits into from
Jun 22, 2024
Merged

Refactor image diff #31444

merged 3 commits into from
Jun 22, 2024

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Jun 21, 2024

And remove some jQuery functions

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 21, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 21, 2024
@github-actions github-actions bot added modifies/templates This PR modifies the template files modifies/js labels Jun 21, 2024
@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jun 21, 2024
@@ -1,4 +1,5 @@
import $ from 'jquery';
import '../vendor/jquery.are-you-sure.js';
Copy link
Member

@silverwind silverwind Jun 21, 2024

Choose a reason for hiding this comment

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

Already imported in web_src/js/features/common-global.js, so this would load it twice needlessly.

Move import to index.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no common-global.js

Copy link
Member

Choose a reason for hiding this comment

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

Where is it currently loaded? Not at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move import to index.js?

I would say no. Only use it where it is really used. Here is initGlobalFormDirtyLeaveConfirm

Copy link
Member

Choose a reason for hiding this comment

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

0678287 regressed it. I recommend moving the import to top of index.js, it's needed on so many pages, we may as well load it everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will rewrite the are-you-sure in next PR, but not in this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-> Fix are-you-sure #31446

Copy link
Contributor Author

@wxiaoguang wxiaoguang Jun 21, 2024

Choose a reason for hiding this comment

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

Maybe we do not need that many PRs, merged #31446 into this since it is simple enough

web_src/js/features/imagediff.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 Jun 21, 2024
boundsInfoAfterHeight.classList.toggle('green', heightChanged);
}
const widthChanged = sizes.imageAfter && sizes.imageBefore && sizes.imageAfter.naturalWidth !== sizes.imageBefore.naturalWidth;
const heightChanged = sizes.imageAfter && sizes.imageBefore && sizes.imageAfter.naturalHeight !== sizes.imageBefore.naturalHeight;
Copy link
Member

Choose a reason for hiding this comment

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

Can these two lines be simplified to sizes.imageAfter?.naturalWidth !== sizes.imageBefore?.naturalWidth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the meaning is different.

@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 22, 2024
@wxiaoguang wxiaoguang enabled auto-merge (squash) June 22, 2024 04:25
@wxiaoguang wxiaoguang merged commit 1a811c0 into go-gitea:main Jun 22, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Jun 22, 2024
@wxiaoguang wxiaoguang deleted the refactor-image-diff branch June 22, 2024 04:54
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 22, 2024
* giteaofficial/main:
  Refactor image diff (go-gitea#31444)
  [skip ci] Updated translations via Crowdin
  Support relative paths to videos from Wiki pages (go-gitea#31061)
  Fix deprecated Dockerfile ENV format (go-gitea#31450)
  README Badge maintenance (go-gitea#31441)
  Improve markdown textarea for indentation and lists (go-gitea#31406)
  Split common-global.js into separate files (go-gitea#31438)
@silverwind
Copy link
Member

silverwind commented Jun 24, 2024

Regression? At https://demo.gitea.com/, maybe it is out of date.

image

@wxiaoguang
Copy link
Contributor Author

No, 621e1ff is a quite old commit

@silverwind
Copy link
Member

a4899ff got deployed and error is gone. Which commit fixed it?

@wxiaoguang
Copy link
Contributor Author

#31444 (review)

@techknowlogick
Copy link
Member

@silverwind the binary release had an issue for a sec, so the updates didn't get pushed to demo. it's since been resolved.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. 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
Development

Successfully merging this pull request may close these issues.

6 participants