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

Segment Editor: Add "Test" button which opens the Visits Log #14884

Merged
merged 11 commits into from Oct 7, 2019
Merged

Conversation

katebutler
Copy link

Fixes #14745

@katebutler katebutler added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Sep 10, 2019
@katebutler katebutler added this to the 3.13.0 milestone Sep 10, 2019
@katebutler katebutler added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Sep 15, 2019
@tsteur
Copy link
Member

tsteur commented Sep 16, 2019

@katebutler could we change the "Test" button to look for example like the "CLOSE" button?

In general not sure about the behaviour. It's great the segment selector stays open so we can create the segment afterwards. But when no visit matches or only one, or when an error happens then nothing can be seen see
image

It's almost as it would need to push the content for the visitor log down only in this special case when clicking on test
image

@diosmosis since you created the issue any thoughts on this?

@diosmosis
Copy link
Member

Not sure how to solve this problem. I suppose alternatively, we could load the segmented visitor log in a modal when test is clicked. Think that would make a difference?

@tsteur
Copy link
Member

tsteur commented Sep 16, 2019

Not sure. Modal likely won't work either as it would still hide the partial result. Only thing I can imagine is pushing the content for the visitor log further down when using the test feature. That should also be easy to do I reckon. There be other options like loading the visits log in the segment editor but this can be tricky... alternatively we could check how many visits match a given segment when clicking on test and show the result as a modal or so. Maybe showing the matching visits in the last X hours be enough?

@diosmosis
Copy link
Member

Modal likely won't work either as it would still hide the partial result.

Wouldn't the modal be on top of everything else? I'm not sure how it would be hidden.

@tsteur
Copy link
Member

tsteur commented Sep 16, 2019

I see you meant showing the visits log in modal not the segment editor? Could work... just not sure what it looks like ... there's minimal space and there's usually a grey background etc.

I now wonder what is the best way for a user to verify it works anyway? Like a number of matching visits? Showing a few visits? How many visits? How can a user be best sure it's working? Showing a number of matching visits plus a few example visits? Is there anything else how a user can be certain the segment is correct?

Would it work to maybe open the test button in a new window? (not best UX though)

@diosmosis
Copy link
Member

Thought about showing the number of visits too. It might work, if the user trusts that the matched visits are correct... but then the segment might work but match the wrong visits and the user would need to see that. It's probably best to show some visits, what do you think?

@tsteur
Copy link
Member

tsteur commented Sep 16, 2019

Yeah I reckon showing some visits is certainly helpful. The segment might still not be expected but I suppose you can never 100% test / verify it and we can always tweak over time if users have some good ideas on how to best validate a segment with additional information.

If MySQL was faster we would directly show for each condition within a segment the number of matched visits and the number of matched visits for the entire segment even before clicking on test... but it wouldn't be fast enough and put quite some load on it.

@diosmosis
Copy link
Member

diosmosis commented Sep 17, 2019

What if we only show the first matched visit? That might be enough to check that it's matching the right criteria. Unless the user uses an 'OR'.

We could also figure this problem out later if it's not a blocker.

EDIT: Disclaimer: I have no idea how important this issue is.

@tsteur
Copy link
Member

tsteur commented Sep 17, 2019

Sounds good. Some simple solution be good

@tsteur
Copy link
Member

tsteur commented Oct 2, 2019

@katebutler could you look at the merge conflicts?

# Conflicts:
#	plugins/SegmentEditor/SegmentEditor.php
#	plugins/SegmentEditor/lang/en.json
@tsteur
Copy link
Member

tsteur commented Oct 2, 2019

@katebutler just gave it a test...

  • When the visitor log popup is open, and I click on escape, then it closes the segment editor but not the popover with the visitor log. Would have expected it the other way around that it closes the visitor log popover. Tested it with other popovers there the esc key doesn't seem to work either. Eg when opening a report, then clicking on the export icon. So maybe the workaround would be to not close the segment editor while the visitor log is open when esc is clicked. This we would likely need to be implemented in the bindEvents method in Segmentation.js

  • Around https://github.com/matomo-org/matomo/pull/14884/files#diff-4e4c0250d8a0ec9660276e396757f6efR760 could we maybe also add another URL parameter like &inPopover=true and then in the visitor log visualisation, in the beforeRender method (https://github.com/matomo-org/matomo/blob/3.12.0-b4/plugins/Live/Visualizations/VisitorLog.php#L115) we could maybe hide the export icon, the paging information, the limit selector, and set the limit selector hard coded to say 5 or 10? like $view->config->show_XYZ= false etc?

Noticed the segment editor disappears in the background when otherwise changing for example the number of shown visits and we wouldn't want to load too many there anyway maybe. Also when clicking on export the popover gets lost otherwise etc.

…closing segment editor when popover is open
@tsteur
Copy link
Member

tsteur commented Oct 4, 2019

@katebutler is it possible to also when the popover is open, and someone clicks in the background, to only close the popover and not the segment editor?

@tsteur tsteur merged commit f371410 into 3.x-dev Oct 7, 2019
@tsteur tsteur deleted the 14745 branch October 7, 2019 01:20
@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Oct 25, 2019
@mattab mattab modified the milestones: 3.13.0, 3.12.0 Oct 25, 2019
@mattab mattab added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. and removed not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Oct 25, 2019
@mattab mattab changed the title Add "test" button to segment editor Segment Editor: Add "Test" button which opens the Visits Log Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add test button to segment editor
4 participants