search

clockify / browser-extension

Clockify Browser Extension
https://clockify.me/chrome-time-tracking
BSD 3-Clause "New" or "Revised" License
185 stars 172 forks source link

gitlab-tasks: fixed problem with small button #82

Closed dpoetzsch closed 4 years ago

dpoetzsch commented 4 years ago

This should fix the issue mentioned here: https://github.com/clockify/browser-extension/pull/80#issuecomment-628653321

aleksandar-olic commented 4 years ago

Awesome! Everything seems to work fine now. I need to do a bit more testing first, but I think we can put this on the Chrome Store live tomorrow.

p.s. I've noticed that the createSmallButton doesn't have the functionality like the createButton (doesn't create stuff or associate tasks and tags).

dpoetzsch commented 4 years ago

p.s. I've noticed that the createSmallButton doesn't have the functionality like the createButton (doesn't create stuff or associate tasks and tags).

Yeah, my bad. When I developed the feature, I was only focussed on our gitlab use case. Do you want me to add this feature to the small button as well? Should be no problem really.

aleksandar-olic commented 4 years ago

If you have the time, that would be great :)

aleksandar-olic commented 4 years ago

Hm, there seems to be an issue in some integrations. Some integrations pass functions as argument in the createButton (like in produck.js or ticktick.js)

dpoetzsch commented 4 years ago

Thanks, I will take a look.

dpoetzsch commented 4 years ago

So I looked at the code and see the problem. I compared it to version 1.8.15 as well, and it seems to me that the project-as-a-function did not work before already (I cannot find any invocation of it). The problem probably never showed up because it just never found a matching project before. I will fix it anyway.

aleksandar-olic commented 4 years ago

Don't worry, it's ok. I've fixed some integrations that use functions as a project, so the problem no longer appears for people who use the integration that work like that.

dpoetzsch commented 4 years ago

But doesn't that change the semantics? Before, the description of the time entry was dynamically calculated when the user clicks on the button (in case of a function). Fixing the integrations means that it is only evaluated once at page load.

Actually I do see a benefit of allowing to pass functions as arguments. For example, if I am in the gitlab issue, change the tags of that issue and then start logging my time the tags will currently not be correctly synced.

So I'd propose to optionally allow each parameter to be a function that will be dynamically invoked at at time of click. Then, the integration can decide what behavior suits best. If you agree I'll create a PR for that.

aleksandar-olic commented 4 years ago

That sounds great! I'll test out the new PR.