Skip to content

Fix reply on code review#10227

Merged
lunny merged 4 commits into
go-gitea:masterfrom
lunny:lunny/fix_reply
Feb 13, 2020
Merged

Fix reply on code review#10227
lunny merged 4 commits into
go-gitea:masterfrom
lunny:lunny/fix_reply

Conversation

@lunny
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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

Comment thread web_src/js/index.js

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

@guillep2k guillep2k Feb 11, 2020

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
Copy Markdown
Member Author

@lunny lunny Feb 11, 2020

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 <art27@cantab.net>
@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 <art27@cantab.net>

Co-authored-by: zeripath <art27@cantab.net>
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 <art27@cantab.net>
@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 <art27@cantab.net>

* Revert unrelated change

* Fix lint

Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: Lauris BH <lauris@nix.lv>
@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

+1 more reviewer

@guillep2k guillep2k guillep2k approved these changes

Reviewers whose approvals may not affect merge requirements

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