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

Real-time visitors widget: Use the same folder color for the same URL #8475

Closed
wants to merge 3 commits into from

Conversation

gboudrias
Copy link

Something like this thread: http://forum.piwik.org/read.php?3,90857

In short, the folder color in the real-time visitor widget doesn't add any information right now.

This PR assigns unique a unique folder color to the most popular pages.

There are only 10 folder colors to choose from right now so the 9 most popular actions get a unique color, and the rest use folder9.png (I think yellow).

Note: There is a bit of computation involved so I don't know what this would do on sites with millions of visitors a day. Probably just take a little while to load.

@gboudrias
Copy link
Author

I really think this is useful and I'm still using it in prod. What can I do to get it in?

@MagicFab
Copy link

@mattab @tsteur @diosmosis what do you think?

@tsteur
Copy link
Member

tsteur commented Nov 15, 2015

I think this could be useful although not 100% sure about it. Would be nice to hear some feedback from actual users. I know there have been requests in the past to assign a fix color to certain pages (or to a group of pages) #6616 but that's all.

I just used it for a while and they only thing that I found annoying that with every second the colors might change completely so the color that was just use for "Page X" might mean 5 seconds something completely else when the ranking of ULRs changes. This is probably more a problem of small websites as it might not happen that often on bigger sites. Here the meaning of a color changed every 5 or 10 seconds. On the other side the colors were always changing before anyway and had no meaning either.

Here's a screenshot
image

It's actually orange that seems to be assigned to pages that are not in the top 9.

I have not really an opinion on this one as I'm not actually using it as much. We could maybe merge it now but we might change it later again to let users assign certain colors. @hpvd @Glisse1 are you using the real time widget regularly? What do you think?

@@ -79,6 +79,39 @@ public function getLastVisitsStart()
$view->idSite = $this->idSite;
$api = new Request("method=Live.getLastVisitsDetails&idSite={$this->idSite}&filter_limit=10&format=php&serialize=0&disable_generic_filters=1");
$visitors = $api->process();
$visits = array();
Copy link
Member

Choose a reason for hiding this comment

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

It took me quite a while to review and understand the code. Maybe you can extract this into maybe two methods or so? Something like getMostUsedUrlsFromVisitors($visitors) and assignColorsToUrls($urls)?

Also the naming confused me a little. Eg $visits does not actually contain visits but a count of how often each URL was used. Also we have $folders which contain actually URLs? Not sure :)

@tsteur
Copy link
Member

tsteur commented Nov 15, 2015

Nice PR otherwise btw 👍

@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Nov 18, 2015
@tsteur
Copy link
Member

tsteur commented Nov 29, 2015

@mattab do you have any opinion on this? (not on the code but on the feature itself)

@mattab
Copy link
Member

mattab commented Dec 21, 2015

@tsteur I don't have strong opinion about this, it'd be OK to merge after code cleanup

@tsteur
Copy link
Member

tsteur commented Dec 21, 2015

@gboudrias do you mind cleaning up the code a little see comments? And then we could merge. Please let us know if you won't find the time to do this, then we'll close the issue

@mattab mattab added this to the 3.0.0 milestone Feb 1, 2016
@mattab
Copy link
Member

mattab commented Feb 1, 2016

Hi @gboudrias - pinging you in case you missed earlier comment

@gboudrias
Copy link
Author

Oh thanks, I actually forgot but I would really like to get this in, I'll implement the changes.

@tsteur tsteur changed the base branch from master to 2.x-dev September 1, 2016 06:40
@mattab
Copy link
Member

mattab commented Sep 23, 2016

Hello @gboudrias

Thank you for this proposed pull request.

Because it was last updated more than one month ago, it is our policy to close pull requests opened for a long time without updates. If you would like to continue work on the pull request, please simply ping us to have it re-opened (after you have pushed a new commit).

We hope you understand this and we look forward to seeing an update from you on this pull request or another one!

We'd be happy to merge this you can apply the feedbak from the review.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants