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
Allow segmented visitor log popup to launch visitor profile #12948
Conversation
…popover management to use window history to retain last opened popover.
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.
Is there currently a way to open the visitor profile from within the segment visitor log? I can't see any link for that |
@sgiehl Should be same place as usual, upper right. The code that changes it is in JS, maybe a cache clear will fix it? |
does
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. |
Yes, basically the same as before. We should be using it in plugins, we shouldn't be using
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 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. |
Cheers, testing it right now. FYI: MediaAnalytics uses |
@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... |
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. |
@tsteur decoupled the popover stack from window history which definitely makes for a better UX, and tested w/ MediaAnalytics. Can you review again? |
@diosmosis do we have any "stacked" popovers to test this behaviour where we close one and open another one? |
@tsteur Only the one I added, ie open segmented visitor log:
then open visitor profile:
|
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@tsteur fixed the issues you identified. |
…ding dialog in odd case where it is created oustide of current viewport.
@mattab There is a behavioral change that you need to approve:
|
@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? |
@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). |
That's great! 🚀 I didn't merge it last minute, but it will make it in the next release 👍 |
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.
Changes:
Piwik_Popover.close()
frombroadcast.propagateNewPopoverParam()
. NowPiwik_Popover.close()
just closes the popover, whilebroadcast.propagateNewPopoverParam()
sets an empty popover query parameter. This means code must usebroadcast.propagateNewPopoverParam(false)
to close a popover that has a popover handler.window.history.back()
to close a popover w/ popover handler. This retains browser history and allows us to "stack" popovers.Tested:
Fixes #7214