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

Fix reply on code review #10227

Merged
merged 4 commits into from
Feb 13, 2020
Merged

Fix reply on code review #10227

merged 4 commits into from
Feb 13, 2020

Conversation

lunny
Copy link
Member

@lunny lunny commented Feb 11, 2020

Should fix #9891

@lunny lunny added this to the 1.12.0 milestone Feb 11, 2020
@codecov-io
Copy link

codecov-io commented Feb 11, 2020 •
edited
Loading

Codecov Report

Merging #10227 into master will decrease coverage by 0.08%.
The diff coverage is 33.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10227      +/-   ##
==========================================
- Coverage   43.63%   43.55%   -0.09%     
==========================================
  Files         576      583       +7     
  Lines       79716    80263     +547     
==========================================
+ Hits        34783    34956     +173     
- Misses      40635    40969     +334     
- Partials     4298     4338      +40
Impacted Files Coverage Δ
routers/org/home.go 65.68% <ø> (-0.34%) ⬇️
modules/structs/hook.go 6.66% <ø> (ø) ⬆️
models/migrations/migrations.go 4.16% <ø> (ø) ⬆️
routers/user/profile.go 50.46% <ø> (-0.24%) ⬇️
routers/init.go 63.04% <0%> (-2.13%) ⬇️
modules/webhook/webhook.go 43.6% <0%> (-1.71%) ⬇️
modules/webhook/feishu.go 0% <0%> (ø)
modules/graceful/manager_unix.go 40.83% <0%> (ø) ⬆️
models/webhook.go 68.7% <0%> (-0.48%) ⬇️
routers/repo/webhook.go 1.27% <0%> (-0.11%) ⬇️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6027809...f349480. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 11, 2020
guillep2k
guillep2k approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

Just one question, anyway LGTM

web_src/js/index.js

window.submitReply = function (btn) {
const form = $(btn).closest('form');
if (form.length > 0 && form.hasClass('comment-form')) {
Copy link
Member

Choose a reason for hiding this comment

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

Why the class/length checks?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just copied from onCodeReviewClicked. :)

@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 Feb 11, 2020
@silverwind
Copy link
Member

silverwind commented Feb 11, 2020 •
edited
Loading

Why not just use <button type="submit">? Inline HTML onclick should be avoided (some reasons) and if it has to be JS, use addEventListener, but I'm pretty certain this issue can be fixed without using any JS because it's a simple form submission that browers can do just fine without JS (unless you want to submit it async, but this case is sync submission).

silverwind
silverwind requested changes Feb 11, 2020
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.

See above.

@lunny
Copy link
Member Author

lunny commented Feb 12, 2020

Why not just use <button type="submit">? Inline HTML onclick should be avoided (some reasons) and if it has to be JS, use addEventListener, but I'm pretty certain this issue can be fixed without using any JS because it's a simple form submission that browers can do just fine without JS (unless you want to submit it async, but this case is sync submission).

I have tried that, but it's not work.

silverwind
silverwind reviewed Feb 12, 2020
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.

Guess I won't block as this is a bugfix, but these form submissions should be cleaned up to work without JS.

6543
6543 approved these changes Feb 12, 2020
View reviewed changes
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

yes a other pr can minimize js for souch things :)

@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 Feb 12, 2020
@lunny
Copy link
Member Author

lunny commented Feb 13, 2020

make L-G-T-M work

@lunny lunny merged commit 1765602 into go-gitea:master Feb 13, 2020
@lunny lunny deleted the lunny/fix_reply branch February 13, 2020 05:22
lunny added a commit to lunny/gitea that referenced this pull request Feb 13, 2020
@lunny @zeripath
Fix reply on code review (go-gitea#10227)
1c24b05 
Co-authored-by: zeripath <[email protected]>
@lunny lunny mentioned this pull request Feb 13, 2020
Merged
lafriks pushed a commit that referenced this pull request Feb 13, 2020
@lunny @zeripath
Fix reply on code review (#10227) (#10257)
987cd27 
Co-authored-by: zeripath <[email protected]>

Co-authored-by: zeripath <[email protected]>
lunny added a commit to lunny/gitea that referenced this pull request Feb 13, 2020
@lunny @zeripath
Fix reply on code review (go-gitea#10227)
4f94142 
Co-authored-by: zeripath <[email protected]>
@lunny lunny mentioned this pull request Feb 13, 2020
Merged
lafriks added a commit that referenced this pull request Feb 14, 2020
@lunny @zeripath @lafriks
Fix reply on code review (#10261)
e786f09 
* Fix branch page pull request title and link error (#10092)

* Fix branch page pull request title and link error

* Fix ui

* Fix reply on code review (#10227)

Co-authored-by: zeripath <[email protected]>

* Revert unrelated change

* Fix lint

Co-authored-by: zeripath <[email protected]>
Co-authored-by: Lauris BH <[email protected]>
@zeripath zeripath added the backport/done All backports for this PR have been created label Feb 15, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@silverwind silverwind silverwind requested changes

@6543 6543 6543 approved these changes

@guillep2k guillep2k guillep2k approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

Reply Button Not Working (Pull Request - File Changed - Split View)
7 participants
@lunny @codecov-io @silverwind @guillep2k @6543 @zeripath @GiteaBot