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

WIP: Add Sec2TrackedTime time format template helper #25213

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

6543
Copy link
Member

@6543 6543 commented Jun 12, 2023

As work-days/-weeks/-years ... differ from country and company, its not practicable to use it as time format

TODOs:

  • migrate trackedTimeComments content -> to be timestamps and use helper to format
  • add UI to change user TT-Max-U setting
  • alter helper to be able to set max unit
    • frontend js helper
    • backend template helper
  • create new JS compontente "tracked time"
  • create a struct to forward user (apperance) settings to frontend (save in window.userSettings or so)
  • migrate in templates to compontente "tracked time"
  • optional: make frontent js tracked-time componente localize

Sponsored by Kithara Software GmbH

6543 added 3 commits June 12, 2023 21:52
As work-days/-weeks/-years ... differ from country and company, its not practicable to use it as time format
@6543 6543 added type/enhancement An improvement of existing functionality outdated/theme/timetracker labels Jun 12, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 12, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 12, 2023
@6543 6543 added this to the 1.21.0 milestone Jun 12, 2023
@6543
Copy link
Member Author

6543 commented Jun 12, 2023

image


image

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 12, 2023
@KN4CK3R
Copy link
Member

KN4CK3R commented Jun 12, 2023

If something took 3 days, why should that differ from country and company? Do you want to use the displayed value for billing?

@6543
Copy link
Member Author

6543 commented Jun 12, 2023

@KN4CK3R yes

@6543
Copy link
Member Author

6543 commented Jun 12, 2023

I personally think that days only are counter-intuitive too. if you think a second time ... well but I'm happy to move it into a server config option through I think we can just separate time formation for tracked-time in any case

Con: we can have large h values
Pro: hours mean the same in any area and can be used to create bills

@KN4CK3R
Copy link
Member

KN4CK3R commented Jun 12, 2023

Then it's debatable if that's the common usecase. Other people want to say "this took me 3 days" without calculating the value. Similar to the "my child is 27 months old" people... well keep your secrets.

@6543
Copy link
Member Author

6543 commented Jun 12, 2023

well was it 3days non stop without sleep ;)

@6543
Copy link
Member Author

6543 commented Jun 12, 2023

well ... so should we go for a server config option?

@silverwind
Copy link
Member

This is only about tracked time, right? I guess you could have a TRACKED_TIME_UNIT option, default to any which renders in the highest unit, and have it settable to days, hours, minutes, seconds which would render in at most that unit. I think you also missed a few places in the UI, e.g. the dropdown in the header bar for active time tracker.

@6543 6543 added the pr/wip This PR is not ready for review label Jun 12, 2023
@6543

This comment was marked as outdated.

@6543 6543 changed the title Add Sec2TrackedTime time format template helper WIP: Add Sec2TrackedTime time format template helper Jun 12, 2023
@wxiaoguang
Copy link
Contributor

For i18n:

For template helper, if it really needs to add a function, it could be put into TimeUtils, like StringUtils/JsonUtils, to avoid bloating the template helpers.

@6543
Copy link
Member Author

6543 commented Jun 13, 2023

well I'll make backend just sends timestamps around ... and conversion can be done in frontend by mentioned browser apis ...

@silverwind
Copy link
Member

I checked relative-time-element which has a format=duration option, but I think it lacks an option to show the value in a defined unit. precision only limits the smallest display unit.

@confusedsushi
Copy link
Contributor

To me, tracked time should always be displayed in hours as highest unit.

The units days, weeks or even years works for something like "I started working on that 3 weeks ago."

But if I say "It took me three days to finish this." It usually means, I got up, had a breakfast, worked some hours, had lunch, worked some hours, had dinner, went to bed and repeated that three times.

In the current form here, 3 days means 72 hours. This is not what I expect.

@silverwind
Copy link
Member

Highest displayed unit should be configurable and default to highest significant unit, I suppose.

@6543
Copy link
Member Author

6543 commented Jun 20, 2023

I'll make it a user setting ... and refactor the code bit by bit to get there :)

  1. make sure backend just pass timestamps and seconds around
  2. move time format into JS frontend
  3. make it configurable and localizable :)

@6543
Copy link
Member Author

6543 commented Jun 20, 2023

next pull towards 1. -> #25392

6543 added a commit that referenced this pull request Jun 23, 2023
this will allow us to fully localize it later

PS: we can not migrate back as the old value was a one-way conversion


prepare for  #25213

---
*Sponsored by Kithara Software GmbH*
@silverwind
Copy link
Member

Still WIP?

@6543
Copy link
Member Author

6543 commented Aug 25, 2023

yes ... Just look at the TODO that's a bigger one :) and I work on it via payed time so it's not as fast as stuff I do in my free time :D (because upstreaming has not the highest priority there)

@6543 6543 modified the milestones: 1.21.0, 1.22.0 Sep 2, 2023
@vsysoev
Copy link

vsysoev commented Dec 2, 2023

Hello, I believe that hour(minutes) are the most universal and cultural independent approach to understand amount of time spent for job to be done. That is why this is truly KISS approach. And not too much job to be done to implement this.

@lafriks
Copy link
Member

lafriks commented Dec 2, 2023

I also agree that for time spent hours should be the highest measurement unit, days and others can be misunderstood and can be confusing

@6543 6543 removed this from the 1.22.0 milestone Dec 3, 2023
@6543
Copy link
Member Author

6543 commented Dec 3, 2023

yes I still want the underlaying code to be cleaner (before i remove the wip) :)

@vsysoev
Copy link

vsysoev commented Dec 3, 2023

yes I still want the underlaying code to be cleaner (before i remove the wip) :)

@6543 because we are doing same job and share same approach, I put my PR #28312 on WIP and wait you to finish yours.

@6543
Copy link
Member Author

6543 commented Dec 3, 2023

Haha ok :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. outdated/theme/timetracker pr/wip This PR is not ready for review size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants