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

Comments on pull files #2583

Closed
wants to merge 1 commit into from
Closed

Conversation

lunny
Copy link
Member

@lunny lunny commented Sep 23, 2017

First step on comment in pull files, and will resolved #124, but still WIP.

@lunny lunny added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Sep 23, 2017
@lunny lunny added this to the 1.x.x milestone Sep 23, 2017
@lunny lunny force-pushed the lunny/commit_line_comment branch from 5e9279a to 21a2b4f Compare September 23, 2017 06:02
@lunny lunny mentioned this pull request Sep 23, 2017
2 tasks
@@ -63,7 +63,7 @@
<link rel="preload" href="{{AppSubUrl}}/vendor/assets/font-awesome/css/font-awesome.min.css" as="style" onload="this.rel='stylesheet'">
<noscript><link rel="stylesheet" href="{{AppSubUrl}}/vendor/assets/font-awesome/css/font-awesome.min.css"></noscript>
<link rel="stylesheet" href="{{AppSubUrl}}/vendor/assets/octicons/octicons.min.css">

<link rel="stylesheet" href="{{AppSubUrl}}/css/main.css">
Copy link
Member

Choose a reason for hiding this comment

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

Why new css file? I think these css additions need to be done in less file and built into minimized index.css

@@ -94,7 +96,8 @@ type Comment struct {
NewTitle string

CommitID int64
Line int64
TreePath string
Line int64 // + is left; - is right
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using +/- we could go with gitea's style and add a type attribute with possible valuesaddition, deletion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't want to add an extra column, so I define the line, if it's greater than zero, it should the previous code line before the pull's commits. If it's less than zero, it should the line on this pull's commits.

Copy link
Member

Choose a reason for hiding this comment

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

If everyone's happy with this I won't obstruct.

Copy link
Member

Choose a reason for hiding this comment

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

IMO, if you use +/-, it should be - for previous line and + for new line.

@@ -1831,3 +1833,290 @@ function toggleStopwatch() {
function cancelStopwatch() {
$("#cancel_stopwatch_form").submit();
}

/* global showdown */
function createCodeComment() {
Copy link
Member

Choose a reason for hiding this comment

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

Please replace javascript comment box creation by using an existing hidden comment template placed in html.
Github does it that way and IMO it's far better.

@@ -62,7 +62,7 @@
<script src="{{AppSubUrl}}/vendor/plugins/emojify/emojify.min.js"></script>
<script src="{{AppSubUrl}}/vendor/plugins/clipboard/clipboard.min.js"></script>
<script src="{{AppSubUrl}}/vendor/plugins/vue/vue.min.js"></script>

<script src="{{AppSubUrl}}/vendor/plugins/showdown/showdown.min.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we already have a markdown converter, since we are doing the exact same thing on issues/comments?

/* global showdown */
function createCodeComment() {
const texts = {
btn: "Comment Code",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use locales for these?

@bkcsoft
Copy link
Member

bkcsoft commented Oct 9, 2017

@lunny Got any screenshots? And are there visible when viewing a specific commit? (like here: https://try.gitea.io/gitea/gitea/commit/b7da5a6cb7c725232c1053a1ca705a6ac0dad467 )

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 9, 2017
@lunny
Copy link
Member Author

lunny commented Oct 10, 2017

@bkcsoft currently only for pull files. For specific commit should be another PR after this merged.

@thehowl
Copy link
Contributor

thehowl commented Apr 22, 2018

What's the status on this?

@thehowl
Copy link
Contributor

thehowl commented Apr 22, 2018

Nevermind, it looks like we're doing this on #3748 instead. Shouldn't this be closed?

@lunny
Copy link
Member Author

lunny commented Apr 23, 2018

@thehowl yes.

@lunny lunny closed this Apr 23, 2018
@lunny lunny removed this from the 1.x.x milestone Apr 23, 2018
@lunny lunny deleted the lunny/commit_line_comment branch August 6, 2018 15:27
@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.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. pr/wip This PR is not ready for review type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Commits and pull files
8 participants