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

Added progressbar for issues (#1146). #3171

Merged
merged 13 commits into from Jan 3, 2018

Conversation

6 participants
@modmew8
Contributor

modmew8 commented Dec 12, 2017

Hi there,

my pull request adds the progressbar for issues as requested in issue #1146. It basically adds three variables to issue.go that hold the information, how many tasks exist, how many are done, and the percentage of the progress.

The values are updated upon creating a new issue and modifying the issues content. The content is parsed using regular expressions, collecting the amount of valid tasks and valid completed tasks.

progressbar

The style is github-alike.

I hope I made everything correct - this is the first pull request in my life :) (please be merciful).

Best regards
modmew8

modmew8 added some commits Dec 12, 2017

Updated the generated index.css.
Signed-off-by: modmew8 <modmew8@gmail.com>
@lunny

This comment has been minimized.

Member

lunny commented Dec 12, 2017

Why stored the percent? I think it's unnecessary.

@lunny lunny added the kind/feature label Dec 12, 2017

@lunny lunny added this to the 1.x.x milestone Dec 12, 2017

@modmew8

This comment has been minimized.

Contributor

modmew8 commented Dec 12, 2017

I could not locate any arithmetic operations in the templates, so I assumed it is task of the models to handle these operations.

As alternative to arithmetic in the templates using go, we could also use css calc():
...style="calc( 100% * {{.Tasksdone}} / {{.Tasks}} );"...

The calculation would be on the clientside this way.

modmew8 added some commits Dec 13, 2017

Merge pull request #2 from modmew8/master
Updating progressbarForIssues with latest master
Removed stored progress percentage and changed it to css calc. Also a…
…dded the issue task progress to the user/dashboard/issues.

Signed-off-by: modmew8 <modmew8@gmail.com>
Removed unnecessary blanks.
Signed-off-by: modmew8 <modmew8@gmail.com>
@modmew8

This comment has been minimized.

Contributor

modmew8 commented Dec 13, 2017

Removed the percent variable and changed the progress bar to use calc().

Formatted the files correctly, fmt-check terminates now without errors.
Signed-off-by: modmew8 <modmew8@gmail.com>
Labels []*Label `xorm:"-"`
MilestoneID int64 `xorm:"INDEX"`
Milestone *Milestone `xorm:"-"`
Tasks int

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 14, 2017

Member

If we're adding new columns to the issue table, then we need a migration.

However, I think the case could be made that we shouldn't store tasks/tasksDone in the database, but instead just calculate them from the Content field.

This comment has been minimized.

@modmew8

modmew8 Dec 14, 2017

Contributor

What do you think about leaving the parsing to the clients? Then we would not have to store anything on the server, and the only files being changed would be the template files (and the css rules). Since it is just a cosmetic addition, the clients could determine the progress themselves upon viewing the issue list - but the server has to provide the content for the clients to parse (additional network load).

I would realize this as a javascript function in the issue list and in the user dashboard issue list.

This comment has been minimized.

@lunny

lunny Dec 14, 2017

Member

@ethantkoenig I don't think so. If the page has 10 issues. We have to parse all the 10 issues' markdowns on the browser one time. And also we need add a javascript markdown library. I don't think this is a good idea.

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 14, 2017

Member

@lunny We don't need to render the issue content as markdown, we just need to look at the raw string (which we load from the DB anyway). The regexps in this PR would not work if the issue contents were rendered.

This comment has been minimized.

@lunny

lunny Dec 15, 2017

Member

@ethantkoenig and another thing, we didn't transfer issue content to client on issue list. If you want client to parse that. We have to transfer the contents. This will also reduce the load speed of the page.

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 15, 2017

Member

I am not suggesting we send the issues' content to the client. That was @modmew8's comment.

I am proposing that we calculate the number of tasks server-side; this can be computed easily with regexps, and does not require rendering the contents as markdown. If our regexps are pre-compiled, I don't think this will be very expensive, since (as I mentioned above) we already load the contents from the DB.

I am hesitant to add new columns to the issue table because I'm afraid that if we will add new columns for every new feature, our schemas will eventually become very complicated and confusing.

MilestoneID int64 `xorm:"INDEX"`
Milestone *Milestone `xorm:"-"`
Tasks int
Tasksdone int

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 14, 2017

Member

TasksDone

@@ -740,7 +743,16 @@ func AddDeletePRBranchComment(doer *User, repo *Repository, issueID int64, branc
func (issue *Issue) ChangeContent(doer *User, content string) (err error) {
oldContent := issue.Content
issue.Content = content
if err = UpdateIssueCols(issue, "content"); err != nil {
regExpTasks := regexp.MustCompile(`(^\s*-\s\[[\sx]\]\s)|(\n\s*-\s\[[\sx]\]\s)`)

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 14, 2017

Member

This is duplicated code, please consider refactoring.

Also, it would be nice if we didn't have to compile the regexp every time.

modmew8 added some commits Jan 2, 2018

Merge branch 'master' of https://github.com/go-gitea/gitea into go-gi…
…tea-master

Merged less changes and updated index.css using make generate-stylesheets.
Merge pull request #4 from go-gitea/master
Merge current Master into fork
Merge pull request #5 from modmew8/master
Merge pull request #4 from go-gitea/master
@codecov-io

This comment has been minimized.

codecov-io commented Jan 2, 2018

Codecov Report

Merging #3171 into master will decrease coverage by <.01%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3171      +/-   ##
==========================================
- Coverage   34.82%   34.82%   -0.01%     
==========================================
  Files         277      277              
  Lines       40263    40271       +8     
==========================================
+ Hits        14023    14025       +2     
- Misses      24181    24186       +5     
- Partials     2059     2060       +1
Impacted Files Coverage Δ
models/issue.go 44.43% <62.5%> (+0.14%) ⬆️
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️
models/repo.go 43.38% <0%> (ø) ⬆️

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 22a7a7e...ae0d1df. Read the comment docs.

Removed variables, made computing the tasks on demand with precompile…
…d regexp.

Signed-off-by: modmew8 <modmew8@gmail.com>
@modmew8

This comment has been minimized.

Contributor

modmew8 commented Jan 2, 2018

I implemented the changes @ethantkoenig suggested, now without new variables in issue, with precompiled regexp and computation on demand when accessing either the issue list or the dashboard issue list.

@tboerger tboerger added lgtm/need 1 and removed lgtm/need 2 labels Jan 2, 2018

@appleboy

This comment has been minimized.

Member

appleboy commented Jan 2, 2018

I restarted the build in Drone.

@lunny lunny modified the milestones: 1.x.x, 1.4.0 Jan 3, 2018

@lunny

This comment has been minimized.

Member

lunny commented Jan 3, 2018

LGTM

@tboerger tboerger added lgtm/done and removed lgtm/need 1 labels Jan 3, 2018

@lunny

This comment has been minimized.

Member

lunny commented Jan 3, 2018

@ethantkoenig needs your approval.

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Jan 3, 2018

@lunny LGTM

@lunny lunny merged commit d996da6 into go-gitea:master Jan 3, 2018

3 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

@lunny lunny referenced this pull request Jan 3, 2018

Closed

Progressbar for issues #1146

@modmew8 modmew8 deleted the modmew8:progressbarForIssues branch Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment