Hello! In this change:
In calendar.js, the logic to fetch new string/update page title has been appended to (done)
propagateNewUrlParams, but in retrospect this would be better in its own function, perhaps still invoked from
Piwik\Plugin\Controller, the logic inside
getLongPrettyDate is currently is copy/pasted from
setGeneralVariablesView. I tried to tease apart the duplicated logic, but it started to become a time sink so I wanted to seek feedback on this approach before spending further time de-duplicating this logic.
Huh, okay - I got my branch updated, no conflicts when merging upstream/master (as outlined in your Contributing to Piwik Core guide).
Shouldn't I be merging upstream/3.x-dev instead though? Quite the conflict there! :) It looks like I should be working in the AngularJS PeriodSelectorController now.
Actually we use
3.x-dev as our master branch, so yes, please rebase your changes on
3.x-dev instead of on
3.x-dev and then force pushed (sorry if I misunderstood your instructions, hopefully no one had work based on my PR branch).
The old calendar.js (now PeriodSelectorController) function (fixed now)
propagateNewUrlParams is no longer being called on page load, so I'll need to figure out another way to fix the issue on page load, but the rest transferred pretty cleanly to the new Angular component.
I went ahead and and refactored the JS logic to its own function
updateDateInTitle, in the
piwik service (organized there so it will be accessible from
piwik.updatePeriodParamsFromUrl). I'll still need to de-dupe the logic in
Piwik\Plugin\Controller::getLongPrettyDate. Thumbs up on this approach?
@spacenate left a comment on this thread: https://github.com/matomo-org/matomo/pull/12222#discussion_r178478479 (which may be hidden by github)
Everything else looks good, so once that's dealt w/, I'll test it and if it's good to go, merge this PR.
I have squashed my commits to make the changes in this PR more clear. Sorry for the delay, and please let me know if there are any other changes you would like to see here!
Oh rats - I think I see why the delay. I forgot to remove "WIP" from the PR title 🤦♂️