Allow segmented visitor log popup to launch visitor profile #12948

diosmosis · 2018-05-21T21:07:58Z

Changes:

Decouple Piwik_Popover.close() from broadcast.propagateNewPopoverParam(). Now Piwik_Popover.close() just closes the popover, while broadcast.propagateNewPopoverParam() sets an empty popover query parameter. This means code must use broadcast.propagateNewPopoverParam(false) to close a popover that has a popover handler.
Use window.history.back() to close a popover w/ popover handler. This retains browser history and allows us to "stack" popovers.
When opening a popover, always close any existing one (so the new modal gets the proper CSS class & positioning).
Show visitor profile link in segmented visitor log.

Tested:

visitor profile through visitor log
segmented visitor log => visitor profile => closing/back button/etc.
row evolution
multi row evolution
transitions
on IE10

Fixes #7214

…popover management to use window history to retain last opened popover.

diosmosis · 2018-05-21T21:57:13Z

Note: might want to put this in 3.6.0 just in case there are hidden issues.

…ad w/ popover does not result in exiting matomo + only do window.history.back() in one place.

sgiehl · 2018-05-22T19:35:48Z

Is there currently a way to open the visitor profile from within the segment visitor log? I can't see any link for that

diosmosis · 2018-05-22T19:41:17Z

@sgiehl Should be same place as usual, upper right. The code that changes it is in JS, maybe a cache clear will fix it?

tsteur · 2018-05-22T21:54:01Z

does broadcast.propagateNewPopoverParameter() still work the same as before? As we are using it in plugins... Not sure I understood the description right.

Now when you close a dialog & hit back, it goes to the URL before the dialog was opened. Before, if you opened row evolution, closed it and hit back, it would reopen the row evolution. This can probably be maintained if it's important.

It would be good to have previous logic 👍

I'm not into the topic so just wondering can we not just close the popover and open another popover when the popover changes? When pressing back, you get to the previous popover?

It seems to me like there shouldn't be many changes needed except listening to changes on the popover search param but I'm really not into the topic.

diosmosis · 2018-05-22T22:22:08Z

does broadcast.propagateNewPopoverParameter() still work the same as before? As we are using it in plugins... Not sure I understood the description right.

Yes, basically the same as before. We should be using it in plugins, we shouldn't be using Piwik_Popover.close() (at least not after this PR).

I'm not into the topic so just wondering can we not just close the popover and open another popover when the popover changes? When pressing back, you get to the previous popover?

That's basically what I did here. Problem is that current code doesn't go back in the history, it changes the popover param adding a new history element. And the code that did that was in Piwik_Popover.close(), so I had to reorganize a bit (otherwise, closing before opening would set a new history item and going back wouldn't work).

There were also some edge cases like entering a URL w/ a popover param (w/ a naive implementation, closing it, window.history.back() takes you out of matomo). And if a user clicks the close button, we need to go back in history via the jquery-ui callback, but if we close the popup programatically (like before opening a new one), we of course don't want to go back. (previously there was no separation of just removing the actual popover & removing the route in the URL).

That said, maybe the PR description is confusing. There aren't that many code changes, might help to just look at those.

tsteur · 2018-05-23T02:44:02Z

Cheers, testing it right now. FYI: MediaAnalytics uses Piwik_Popover.close() so not sure if something will break there and whether we can keep it compatible with both 3.5.1+ and older versions.
From what I see there is not yet a way to trigger two popovers right? Tried to find a way to trigger it...

diosmosis · 2018-05-23T03:03:19Z

@tsteur depends on whether it uses the popover= param or not. If it does use the param then it won’t end up removing the parameter. Not sure how to get around that if that’s the case...

diosmosis · 2018-05-23T03:39:39Z

From looking at the MediaAnalytics plugin, it looks like it's use of close() should work ok. It might result in going back to the non segmented popover, but I think that's a good thing? I'll have to try and test locally.

…d to make that work.

diosmosis · 2018-05-24T02:07:43Z

@tsteur decoupled the popover stack from window history which definitely makes for a better UX, and tested w/ MediaAnalytics. Can you review again?

tsteur · 2018-05-24T20:55:26Z

@diosmosis do we have any "stacked" popovers to test this behaviour where we close one and open another one?

diosmosis · 2018-05-24T21:02:14Z

@tsteur Only the one I added, ie open segmented visitor log:

<removed screenshot>

then open visitor profile:

<removed screenshot>

tsteur · 2018-05-24T22:13:37Z

plugins/CoreHome/javascripts/dataTable_rowactions.js

@@ -373,7 +373,7 @@ DataTable_RowActions_RowEvolution.prototype.showRowEvolution = function (apiMeth
            // remember label for multi row evolution
            box.find('.rowevolution-startmulti').click(function () {
                Piwik_Popover.onClose(false); // unbind listener that resets multiEvolutionRows
-                Piwik_Popover.close();
+                broadcast.propagateNewPopoverParameter(false);


Would it be possible that Piwik_Popover.close() calls this method or not easily doable? I feel like it would be better understandable to have only Piwik_Popover.close() which is quite expressive and something you would expect to call in order to close the popup but as only we are supposed to use it, it might be fine if not doable.

Or maybe we could have another method like Piwik_Popover.closeAll() or something? I'm not quite sure what the difference is between Popover.close() and broadcast.propagateNewPopoverParameter(false); and when to call any of them.

Not really, at least not w/o a fair amount of work. The problem is that it is used by, eg, popover-handler.js to close a popover while in the middle of a popover change when we just want to remove the popover element, but not the popover parameter.

Ie, what happens:

broadcast.propagateNewPopoverParameter(false) is called

this sets the popover= param to a changed value (eg, the last value in the stack)

$locationChangeSuccess is invoked

popover-handler.directive.js closes the existing popover by calling Piwik_Popover.close()

If Piwik_Popover.close() calls broadcast.propagateNewPopoverParameter(false); at this point, the popover parameter that was added gets removed.

I think a lot of this code needs to be reworked later.

sounds good to rework it later. Just out of curiosity when needs broadcast.propagateNewPopoverParameter(false); called and when Piwik_Popover.close() and what do they do differently? be good to explain this somewhere or have a more "speakable" method name for it.

Originally broadcast.propagateNewPopoverParameter(false); unset the popover= query parameter & Piwik_Popover.close() would close the popup and then call broadcast.propagateNewPopoverParameter(false);.

Now, the responsibilities are:
broadcast.propagateNewPopoverParameter(false);: removes the popover query parameter (& triggers the $locationChangeSuccess event through a digest)
Piwik_Popover.close(): removes modal (ie, removes the DOM element & cleans up)

So one affects the element and another affects the URL.

diosmosis · 2018-05-24T23:23:15Z

@tsteur fixed the issues you identified.

…ding dialog in odd case where it is created oustide of current viewport.

diosmosis · 2018-05-25T00:59:35Z

@mattab There is a behavioral change that you need to approve:

On clicking outside a modal, every popover in the stack is now closed (so it exits out every stacked popover). (so if a user opens the segmented visitor log => visitor profile => clicks outside dialog, it just closes the visitor profile. contrast w/ segmented visitor log => visitor profile => clicks close button => opens segmented visitor log.)

mattab · 2018-05-25T06:27:00Z

@diosmosis is it possible to come back to the segmented visitor log, from the visitor profile, in another way? it seems useful that one could easily browse several visitor profiles by going back to the segmented visitor log and clicking on another profile. Would it be easily do-able or is it a technical thing that it's not possible?

diosmosis · 2018-05-25T10:34:03Z

@mattab yes it’s doable, you have to click the ‘x’ close button instead of outside the dialog (or hit the browser back button). Clicking outside on the other hand closes all of them (which I and tsteur think is more user friendly).

mattab · 2018-05-25T11:04:19Z

That's great! 🚀

I didn't merge it last minute, but it will make it in the next release 👍

diosmosis · 2018-05-27T20:43:29Z

Will merge this now that 3.5.1 is out.

…rg#12948) * Allow segmented visitor log popup to launch visitor profile & modify popover management to use window history to retain last opened popover. * Keep track of popovers opened so closing popover when initial page load w/ popover does not result in exiting matomo + only do window.history.back() in one place. * Decouple popover stack from window history because its too complicated to make that work. * Reset popover stack as precaution. * Clean up visitor log tooltips on destruction. * Fix widgetized popover open/close. * Another precaution against zombie tooltips. * Reset popover stack if user clicks outside of modal and scroll to loading dialog in odd case where it is created oustide of current viewport. * Update expected screenshots.

Allow segmented visitor log popup to launch visitor profile & modify …

4b47a01

…popover management to use window history to retain last opened popover.

diosmosis added this to the 3.5.1 milestone May 21, 2018

Keep track of popovers opened so closing popover when initial page lo…

c5624ad

…ad w/ popover does not result in exiting matomo + only do window.history.back() in one place.

diosmosis added 2 commits May 23, 2018 18:05

Merge branch '3.x-dev' into 7214-stackable-popovers

f374bc4

Decouple popover stack from window history because its too complicate…

587cc34

…d to make that work.

Reset popover stack as precaution.

e006d63

tsteur reviewed May 24, 2018

View reviewed changes

diosmosis added 2 commits May 24, 2018 15:55

Clean up visitor log tooltips on destruction.

eba2911

Fix widgetized popover open/close.

62d067e

Another precaution against zombie tooltips.

2bac5c1

Reset popover stack if user clicks outside of modal and scroll to loa…

7fb66bf

…ding dialog in odd case where it is created oustide of current viewport.

Update expected screenshots.

8582d11

mattab modified the milestones: 3.5.1, 3.6.0 May 25, 2018

Merge branch '3.x-dev' into 7214-stackable-popovers

83c395c

diosmosis merged commit 02614a3 into 3.x-dev May 27, 2018

diosmosis deleted the 7214-stackable-popovers branch May 27, 2018 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow segmented visitor log popup to launch visitor profile #12948

Allow segmented visitor log popup to launch visitor profile #12948

diosmosis commented May 21, 2018 •

edited

diosmosis commented May 21, 2018

sgiehl commented May 22, 2018

diosmosis commented May 22, 2018

tsteur commented May 22, 2018

diosmosis commented May 22, 2018 •

edited

tsteur commented May 23, 2018

diosmosis commented May 23, 2018

diosmosis commented May 23, 2018

diosmosis commented May 24, 2018

tsteur commented May 24, 2018

diosmosis commented May 24, 2018 •

edited

tsteur May 24, 2018

tsteur May 24, 2018

diosmosis May 24, 2018 •

edited

tsteur May 24, 2018

diosmosis May 25, 2018

diosmosis commented May 24, 2018

diosmosis commented May 25, 2018

mattab commented May 25, 2018

diosmosis commented May 25, 2018

mattab commented May 25, 2018

diosmosis commented May 27, 2018

Allow segmented visitor log popup to launch visitor profile #12948

Allow segmented visitor log popup to launch visitor profile #12948

Conversation

diosmosis commented May 21, 2018 • edited

diosmosis commented May 21, 2018

sgiehl commented May 22, 2018

diosmosis commented May 22, 2018

tsteur commented May 22, 2018

diosmosis commented May 22, 2018 • edited

tsteur commented May 23, 2018

diosmosis commented May 23, 2018

diosmosis commented May 23, 2018

diosmosis commented May 24, 2018

tsteur commented May 24, 2018

diosmosis commented May 24, 2018 • edited

tsteur May 24, 2018

Choose a reason for hiding this comment

tsteur May 24, 2018

Choose a reason for hiding this comment

diosmosis May 24, 2018 • edited

Choose a reason for hiding this comment

tsteur May 24, 2018

Choose a reason for hiding this comment

diosmosis May 25, 2018

Choose a reason for hiding this comment

diosmosis commented May 24, 2018

diosmosis commented May 25, 2018

mattab commented May 25, 2018

diosmosis commented May 25, 2018

mattab commented May 25, 2018

diosmosis commented May 27, 2018

diosmosis commented May 21, 2018 •

edited

diosmosis commented May 22, 2018 •

edited

diosmosis commented May 24, 2018 •

edited

diosmosis May 24, 2018 •

edited