@bx80 opened this Pull Request on September 30th 2021 Contributor

Description:

Fixes #10291

For charts where the last point on the horizontal axis has a date that is either today or in the future, then that segment of the line will be plotted in a dashed style to indicate that data for that period will be incomplete.

image

Review

@tsteur commented on October 3rd 2021 Member

ImageGraph we could have a separate issue for it maybe as most important be the UI (re 80/20 rule). cc @mattab

@mattab commented on October 3rd 2021 Member

Sounds good to have a separate issue for it, as the most pain is felt in the UI :+1:

@mattab commented on October 4th 2021 Member

Looks great. Feedback:

  • As people will wonder what the dashed line means, maybe we could edit the tooltip on the last period and mention (incomplete period.) under the date in the tooltip? or some other better wording?
@tsteur commented on October 8th 2021 Member

@bx80 looks like there might be changes from another PR in there from exclude parameters?

@bx80 commented on October 10th 2021 Contributor

@tsteur Because the periods service had been converted to vue while I was working on this issue I merged 4.x-dev into this branch to use those changes. The 4.x-dev branch now includes the exclude parameters change which I think is why those changes are appearing in this branch. I should perhaps have used rebase instead of merge?

@justinvelluppillai commented on October 11th 2021 Contributor

@bx80 merge should normally be fine. I don't see any changes from your other PR here, has it resolved itself meantime or did I miss something? It does have some conflicts now though.

@tsteur commented on October 11th 2021 Member

Looks all good to me and works @bx80 . Could we create a UI test though showing the dotted line that this works?

@bx80 commented on October 11th 2021 Contributor

It might be tricky to do a UI test as the dotted line will only show if the current period contains today, the check for whether it is today or not is being done in the periods service. I've tried overriding the Date.now() function prior to loading the page in a UI test, which is how the periods service tests work. but those tests directly call the periods calls, and that approach doesn't seem to work here, as I'm guessing a new javascript vm is being spawned by the test system when loading the page. Is there a usual way to fake today's date in javascript for a UI test?

@bx80 commented on October 11th 2021 Contributor

@justinvelluppillai The outstanding conflicts are all from the vue.js typescript build output, I suppose I'll need to merge 4.x-dev into this branch again to pickup whatever has changed CoreHome typescript in 4.x-dev and then do another ./console build:vue, then commit an updated set of UMDs, then resolve this branch conflict by preferring them over the current 4.x-dev versions (assuming 4.x-dev hasn't changed again in the meantime) Seems like a 'build vue' github action like we have for 'build js' could save a lot of time going forward.

@tsteur commented on October 12th 2021 Member

@bx80 I was hoping that it might work similarly to https://github.com/matomo-org/matomo/blob/4.5.0/tests/UI/specs/OnlyRawDataNotification_spec.js#L11-L13

I'm thinking if we could use idSite=1&period=year&date=today as URL and in the fixture track a wee bit of data for this year, and some data for the past year, then this might work? We would probably just need to update the image once a year but otherwise the screenshot might not change.

Be great to check if that could work. It might be even easiest to create a new spec file just for this and a dedicated fixture where we track bit of data for this and for the previous year. Since it's something that could easily regress it be very valuable to have a screenshot test for it.

@bx80 commented on October 13th 2021 Contributor

@tsteur That approach worked well, thank you. I've committed a new UI test and dedicated fixture to create a few visits in the current and previous year and compare the visitors overview chart.

This Pull Request was closed on October 14th 2021
Powered by GitHub Issue Mirror