Skip to content
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

Add new transitions report #13475

Merged
merged 24 commits into from Dec 11, 2018
Merged

Add new transitions report #13475

merged 24 commits into from Dec 11, 2018

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Sep 25, 2018

As you can maybe see in the first commit I tried to first make it work as a report with a visualization and including related report etc but couldn't get it to work. There were various challenges. So made it a widget instead. Was still quite tricky to get it to work and is a bit messy since it was all hard adjusted to be a popover.

Looks like this:
image

No data screen:
image

Menu:
image

There is no search in the label field for now. It's only our regular select. Should do for now I'd say and at some point need to add support for autocomplete or so.

I'll try to add some UI tests if that's looking good @mattab

fix #12865

@tsteur tsteur added this to the 3.7.0 milestone Sep 25, 2018
@tsteur tsteur added Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Sep 25, 2018
@diosmosis
Copy link
Member

Testing locally noticed these issues:

  1. the report doesn't seem to be centered, might look better if it was

image

  1. If I click in the transitions report, I see a popup like this

image

and then the report is empty

image

  1. the select dropdown is under the report

image

@diosmosis
Copy link
Member

@mattab UI needs approval

self.isEnabled = false;
self.actionName = '';
self.actionNameOptions.push({key: '', value: translate('CoreHome_ThereIsNoDataForThisReport')});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super important, but I think this method could be split up for better readability, if desired.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do 👍

@tsteur
Copy link
Member Author

tsteur commented Dec 2, 2018

the report doesn't seem to be centered, might look better if it was

I didn't manage to center it but I actually think it's fine like this anyway. Feel free to give it a try. It's so hard to do much there I think cause the transitions JS is quite complex and built for popup etc.

If I click in the transitions report, I see a popup like this

had actually no idea it was clickable... i think the JS is not able to render 2 transition reports on the same page or it's trying to alter something.
It should more or less work now. If there's still a problem I will need to disable the onclick functionality in this report.

the select dropdown is under the report

should be fixed

also applied the other feedback @diosmosis

@diosmosis
Copy link
Member

LGTM just need UX approval from @mattab & UI tests

@tsteur
Copy link
Member Author

tsteur commented Dec 6, 2018

fyi: merged 3.x-dev into this PR. Fixed system test & ui tests. Added UI tests (need to update again once the build has finished). The UI test show something funny for transitions page titles but checked and this seems to be actually a general issue of transition, not related to the integration etc. UI should be fine for now, don't think we need to wait for it to merge re @mattab

@tsteur
Copy link
Member Author

tsteur commented Dec 7, 2018

Tests should be fixed now.

@diosmosis
Copy link
Member

diosmosis commented Dec 8, 2018

Was going to merge, but the transitions widget looks a bit off:

image

Guess the page title shouldn't display in the dashboard.

I think something in the code is also causing the UI test on travis to have issues (works fine for me locally): https://builds-artifacts.matomo.org/matomo-org/matomo/12865/31592/UIIntegrationTest_dashboard3.png

Think it might have something to do w/ the width of an element.

@tsteur
Copy link
Member Author

tsteur commented Dec 10, 2018

@diosmosis I think I fixed the remaining issues this morning

@mattab
Copy link
Member

mattab commented Dec 10, 2018

Looks really good overall, it's nice to know more users will discover this feature.
Feedback:

  • In the footer message eg. Simply hover a row in any of these reports and click on the transition icon to launch it. could maybe add the Transitions icon in the text itself.
  • Add a "Help text" for the report, that 1) gives a quick explanation of what the report is, and 2) links to: https://matomo.org/docs/transitions/
  • When viewing the transitions for a site, and changing site, then I get error:
Please specify a value for 'actionName'.#0 /core/API/Proxy.php(157): Piwik\API\Proxy->getRequestParametersArray(Array, Array)#1 /core/Context.php(28): Piwik\API\Proxy->Piwik\API\{closure}()#2 /core/API/Proxy.php(323): Piwik\Context::executeWithQueryParameters(Array, Object(Closure))#3 /core/API/Request.php(263): Piwik\API\Proxy->call('\\Piwik\\Plugins\\...', 'getTransitionsF...', Array)#4 /plugins/API/Controller.php(41): Piwik\API\Request->process()#5 [internal function]: Piwik\Plugins\API\Controller->index()#6 /core/FrontController.php(564): call_user_func_array(Array, Array)#7 /core/FrontController.php(152): Piwik\FrontController->doDispatch('API', false, Array)#8 /core/dispatch.php(34): Piwik\FrontController->dispatch()#9 /index.php(27): require_once('/home/matt/dev/...')#10 {main}
  • in case it's possible, would be nice if we could re-draw the Transitions report when the browser window is resized (currently the Transitions report is not re-drawn and will overflow or look small)

@tsteur
Copy link
Member Author

tsteur commented Dec 10, 2018

  • added the icon
  • added help text & link
  • couldn't reproduce an error when switching site.... added some code maybe it helps
  • No we cannot resize it, it's a "miracle" it was possible to embed it at all

@diosmosis diosmosis merged commit a06e2d5 into 3.x-dev Dec 11, 2018
@diosmosis diosmosis deleted the 12865 branch December 11, 2018 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New report "Transitions" available under the "Actions" category
3 participants