@tsteur opened this Pull Request on September 25th 2018 Member

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

@diosmosis commented on December 1st 2018 Member

Testing locally noticed these issues:

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

image

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

image

and then the report is empty

image

3) the select dropdown is under the report

image

@diosmosis commented on December 1st 2018 Member

@mattab UI needs approval

@tsteur commented on December 2nd 2018 Member

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 commented on December 3rd 2018 Member

LGTM just need UX approval from @mattab & UI tests

@tsteur commented on December 6th 2018 Member

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 commented on December 7th 2018 Member

Tests should be fixed now.

@diosmosis commented on December 8th 2018 Member

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 commented on December 10th 2018 Member

@diosmosis I think I fixed the remaining issues this morning

@mattab commented on December 10th 2018 Member

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'.<a href='/0'>#0</a> /core/API/Proxy.php(157): Piwik\API\Proxy->getRequestParametersArray(Array, Array)<a href='/1'>#1</a> /core/Context.php(28): Piwik\API\Proxy->Piwik\API\{closure}()<a href='/2'>#2</a> /core/API/Proxy.php(323): Piwik\Context::executeWithQueryParameters(Array, Object(Closure))<a href='/3'>#3</a> /core/API/Request.php(263): Piwik\API\Proxy->call('\\Piwik\\Plugins\\...', 'getTransitionsF...', Array)<a href='/4'>#4</a> /plugins/API/Controller.php(41): Piwik\API\Request->process()<a href='/5'>#5</a> [internal function]: Piwik\Plugins\API\Controller->index()<a href='/6'>#6</a> /core/FrontController.php(564): call_user_func_array(Array, Array)<a href='/7'>#7</a> /core/FrontController.php(152): Piwik\FrontController->doDispatch('API', false, Array)<a href='/8'>#8</a> /core/dispatch.php(34): Piwik\FrontController->dispatch()<a href='/9'>#9</a> /index.php(27): require_once('/home/matt/dev/...')<a href='/10'>#10</a> {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 commented on December 10th 2018 Member
  • 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
Powered by GitHub Issue Mirror