-
Notifications
You must be signed in to change notification settings - Fork 73
EditCounter: make the day count in the timecard update with timezone change #497
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
Conversation
by replacing timeCardDatasets line 339 with chart.data.datasets. timeCardDatasets is the initial value, whereas chart.data.datasets is the updated value (timeCardDatasets doesn't get updated because we do chart.data.datasets = chart.data.datasets
|
There's a build step that needs to be ran locally. Let me see if I can take care of that for you. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #497 +/- ##
============================================
- Coverage 71.30% 70.96% -0.35%
- Complexity 1275 1293 +18
============================================
Files 46 46
Lines 3792 3892 +100
============================================
+ Hits 2704 2762 +58
- Misses 1088 1130 +42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MusikAnimal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully an easy fix.
|
This is weird. I was afraid of such issues, so I made a quick test in the console: And it worked, it did output Perhaps the callback is called during initalisation of the variable, too. Eh, whatever. |
by using window.chart ? chart.data.datasets : timeCardDatasets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer errors, but unfortunately it doesn't work. The values on the right stay the same. I did a little debugging and it seems chart.data.datasets apparently isn't updated (?)
I can try to debug more, but this might be easier if we can get your dev environment set up. I'm happy to help with that. If you a Wikimedia developer account, and you've used Toolforge before (i.e. have SSH keys set up), you're already most of the way there. Development docs are at https://www.mediawiki.org/wiki/XTools/Development
|
Yeah, I really should take time and set up a dev environment. Reading the list for the first time, I think I have everything except the git setup. But I'll probably manage. |
|
I'd appreciate if you could lend a hand. I think I've done everything right, but I'm getting an exception: With Being on a weird linux distribution and not having updated my package list for a year or two doesn't help, but normally I have versions >= the requirements. |
For Linux, I think you should be able to get by with just the PHP adapter: We can also chat in realtime, either on IRC or Discord. I'm MusikAnimal in both places. We have a two-year old PR to create a Docker container (#441). I wish I knew enough about Docker to get that finished… |
|
(Left a friend request at discord.) On having done to setup the tunnels (I added -f to make it run in the background, else it actually ssh'd into toolforge and there I can't install anything, and -N because else it complains). |
|
There we go! Found the issue: even if we change the y property of an hour bubble, it's still classified within its original day; so I just filtered all hours to keep only those that in the end land in the right day. As we've got a fixed N of 7*24=168 that really isn't big, it should be fine. |
MusikAnimal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! 😄
n.b. use npm run build to compile the assets before committing, which will remove the old ones automatically. I know this is annoying but we don't have a build pipeline for deployments yet.
You can also use npm run watch during development, so you don't have to manually rebuild constantly.
|
I did figure it out eventually; my issue is I haven't figured out how to commit with git yet (yes, really 🤡), so I'm using the github UI that only lets you change a few files at a time. |
Problem: when the user changes the timezone, currently the sums at the right don't get updated.
Solution: replacing timeCardDatasets in the callback line 339 with chart.data.datasets.
timeCardDatasets is the initial value, whereas chart.data.datasets is the updated value (timeCardDatasets doesn't get updated because we do chart.data.datasets = chart.data.datasets.map(...))
(And this time it's in a language I've written a lot in.
And of course, I forgot to link to the bug in the commit again...)
Bug: T385962