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

Display current stopwatch in navbar #14122

Merged
merged 19 commits into from Jan 21, 2021
Merged

Conversation

noerw
Copy link
Member

@noerw noerw commented Dec 23, 2020

Adds link to currently tracking issue into navbar
This also requires to extend the API response for stopwatches with the current tracked duration

timetrack

fixes #4378

@noerw
Copy link
Member Author

noerw commented Dec 23, 2020

Possible alternative UI (but I don't really have the patience to fix this layout proper 🙃):

Screenshot from 2020-12-23 10-11-27

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 23, 2020
@noerw noerw added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Dec 23, 2020
@lunny
Copy link
Member

lunny commented Dec 23, 2020

But if you start watch serval issues, how to display them?

@silverwind
Copy link
Member

silverwind commented Dec 23, 2020

I would just display a single clock icon and make a hover dialog similar to what was done in #13977 to display the running trackers. Keep colors neutral, e.g. link to issue left aligned, time right-aligned.

@6543 6543 added this to the 1.14.0 milestone Dec 23, 2020
@bobemoe
Copy link
Contributor

bobemoe commented Dec 23, 2020

Since #10533 there can be only one stopwatch.

@lunny
Copy link
Member

lunny commented Dec 24, 2020

Since #10533 there can be only one stopwatch.

Missed that.

@bobemoe
Copy link
Contributor

bobemoe commented Dec 24, 2020

With this new functionality it would make managing multiple stopwatches much easier, I wouldn't be opposed to reverting #10533, or maybe implementing it as an config option as suggested in that PR? Personally I cant really see a use case for multiple tracking.

Could the current sidebar stopwatch message be removed once this navbar version is implemented? Especially if multiple stopwatches get reimplemented as its a bit confusing, see #10529

@lunny
Copy link
Member

lunny commented Dec 25, 2020

With this new functionality it would make managing multiple stopwatches much easier, I wouldn't be opposed to reverting #10533, or maybe implementing it as an config option as suggested in that PR? Personally I cant really see a use case for multiple tracking.

Could the current sidebar stopwatch message be removed once this navbar version is implemented? Especially if multiple stopwatches get reimplemented as its a bit confusing, see #10529

I agree with you. I don't think #10533 needs to be reverted. A config option will make the things complicated.

@lunny
Copy link
Member

lunny commented Dec 25, 2020

CI failed is related.

@noerw
Copy link
Member Author

noerw commented Dec 25, 2020

I changed the UI with suggestions by @silverwind, I think it's good now:
Peek 2020-12-25 05-59

@bobemoe As long as there is a single timer, that notice should stay. Also I can certainly see usecases for multiple timers: tracking a specific issue for billing, and the general topic you're spending time with for personal overview. If we re-enable that, this will be in a separate PR.

edit: not sure what's going on with the test failure, seems unrelated to my changes? 🤔

@silverwind
Copy link
Member

Looks nice. Can we maybe also live-update the displayed time in the dialog, using JS loop that updates it every second?

@noerw
Copy link
Member Author

noerw commented Jan 10, 2021

I added clientside live updates to the displayed time, but it's somewhat janky due to client/serverside synchroniziation / drift.
If you think that it's still good-enough, this PR is ready from my side.

@codecov-io
Copy link

codecov-io commented Jan 10, 2021

Codecov Report

Merging #14122 (34d03ae) into master (56a8929) will increase coverage by 0.00%.
The diff coverage is 68.96%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #14122   +/-   ##
=======================================
  Coverage   41.86%   41.87%           
=======================================
  Files         745      745           
  Lines       79837    79866   +29     
=======================================
+ Hits        33421    33440   +19     
- Misses      40893    40899    +6     
- Partials     5523     5527    +4     
Impacted Files Coverage Δ
routers/repo/issue_stopwatch.go 49.01% <59.09%> (+7.64%) ⬆️
models/issue_stopwatch.go 71.42% <100.00%> (+1.05%) ⬆️
modules/convert/issue.go 87.59% <100.00%> (+0.19%) ⬆️
routers/routes/macaron.go 93.26% <100.00%> (+<0.01%) ⬆️
modules/context/context.go 55.31% <0.00%> (-2.66%) ⬇️
routers/repo/repo.go 30.12% <0.00%> (-2.52%) ⬇️
modules/log/file.go 73.60% <0.00%> (-1.61%) ⬇️
models/issue.go 57.14% <0.00%> (+0.33%) ⬆️
routers/repo/view.go 41.58% <0.00%> (+0.62%) ⬆️
modules/queue/workerpool.go 60.00% <0.00%> (+1.22%) ⬆️
... and 1 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 56a8929...34d03ae. Read the comment docs.

@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 Jan 10, 2021
@techknowlogick techknowlogick added the topic/ui Change the appearance of the Gitea UI label Jan 12, 2021
@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 Jan 21, 2021
@6543 6543 merged commit b5570d3 into go-gitea:master Jan 21, 2021
@6543 6543 deleted the stopwatch-notif branch January 21, 2021 14:52
@go-gitea go-gitea locked and limited conversation to collaborators Mar 11, 2021
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. topic/ui Change the appearance of the Gitea UI 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.

Feature enhancement: Dashboard should show active time tracker
9 participants