@spacenate opened this Pull Request on October 24th 2017 Contributor

Fixes #12162

Hello! In this change:

  • Added a new protected method getLongPrettyDate to Piwik\Plugin\Controller.
  • Added a new public method getDateString to Piwik\Plugins\CoreHome\Controller.
  • Added a new XHR request to the calendar.js module in CoreHome, to get a new "long pretty date string" when the user has updated the date or range.

In calendar.js, the logic to fetch new string/update page title has been appended to propagateNewUrlParams, but in retrospect this would be better in its own function, perhaps still invoked from propagateNewUrlParams? (done)

In 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.

Your thoughts?

@spacenate commented on October 24th 2017 Contributor

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.

@sgiehl commented on October 24th 2017 Member

Actually we use 3.x-dev as our master branch, so yes, please rebase your changes on 3.x-dev instead of on master

@spacenate commented on October 27th 2017 Contributor

Rebased 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 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. (fixed now)

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?

@diosmosis commented on April 17th 2018 Member

@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.

@spacenate commented on May 14th 2018 Contributor

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!

@spacenate commented on June 25th 2018 Contributor

Oh rats - I think I see why the delay. I forgot to remove "WIP" from the PR title 🤦‍♂️

@diosmosis commented on June 25th 2018 Member

Thanks again for the contribution @spacenate!

@mattab commented on August 28th 2018 Member

Very useful improvement @spacenate :+1:

(fyi created this follow up issue: Page title in Tag Manager, Help page, Admin pages show the date but there is no calendar in the page #13353 )

This Pull Request was closed on June 25th 2018
Powered by GitHub Issue Mirror