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

Remove Stats from Toolbar #3040

Merged
merged 6 commits into from
Feb 12, 2021
Merged

Remove Stats from Toolbar #3040

merged 6 commits into from
Feb 12, 2021

Conversation

yakkomajuri
Copy link
Contributor

Changes

Please describe.
If this affects the frontend, include screenshots.

PRs over issues hence this.

This removes the 'Stats' button from the Toolbar.

Screenshot 2021-01-21 at 16 46 42

Rationale

We never actually built out this functionality and it confuses users + creates an expectation that we will build this (which doesn't seem to be in the plans any time now).

Would be happy to consider other options like putting it behind a feature flag, but just wanted to spark the discussion about this.

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests

@timgl timgl temporarily deployed to posthog-remove-stats-qnodychx6 January 21, 2021 19:53 Inactive
@Twixes
Copy link
Collaborator

Twixes commented Jan 21, 2021

Ohh, nice, it's a yes from me, though let's wait for Marius.
Honestly I don't even know what these Stats were supposed to be. 🙈

@mariusandra
Copy link
Collaborator

You were supposed to get a quick google analytics style view of what has been happening on the page. E.g. a 7-day line graph showing visitors, a pie chart with countries, etc. Basic web analytics. Maybe even real time stats a'la Google Analytics / Chartbeat if we can figure out a nice way to do that. (That's another subcategory of products to enter)

Perhaps it can make sense to put a few days of focus on the toolbar next sprint? E.g. fix some things to make it less annoying, actually show some stats, improve the actions views and click handling, etc?

If not, removing this is fine. However then I'd 1) remove all the stats code (some stuff still in logics and elsewhere that I haven't checked), 2) move the other icons down a bit.

@mariusandra
Copy link
Collaborator

Out of scope for here, but let's replace that "stats" box with a "feature flags" box that lets you toggle them in/out one by one. This will make the posthog toolbar into a real toolbar for your site!

@Twixes
Copy link
Collaborator

Twixes commented Feb 9, 2021

Can we go ahead with this?

@jamesefhawkins
Copy link
Collaborator

Definitely in favor of removing for now. If there's a solid argument to otherwise enhance, I'm open but I don't see that currently.

The feature flags idea is super awesome, but again, probably needs a few sprints first on some of our quality-focussed work to make sense to prioritize.

@yakkomajuri
Copy link
Contributor Author

Agree we should - will remove the remaining logic code

@yakkomajuri
Copy link
Contributor Author

@mariusandra would particularly like your input on how much to pull the buttons down. I liked the heatmap being aligned with the hedgehog, but the best thing here is to get your opinion on what that top value should be.

Also probably should get your blessing to merge this anyway 😁

@yakkomajuri
Copy link
Contributor Author

@mariusandra ready for review

@@ -107,7 +107,7 @@ export function Circle({
{
zIndex,
position: 'absolute',
top: 0,
top: 15,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, this might have some unintended consequences. Best to consider this <Circle /> a library that we use and only control it from the outside.

The right place to update the coordinates is here:

However, it might just be fine to leave them as they are since we planned to do some bigger changes to the toolbar soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it - will revert to original 0 here.

@mariusandra
Copy link
Collaborator

Looks good then! Feel free to merge whenever github lets you :)

@yakkomajuri yakkomajuri merged commit cb979c5 into master Feb 12, 2021
@yakkomajuri yakkomajuri deleted the remove-stats branch February 12, 2021 13:36
@mariusandra mariusandra mentioned this pull request Jun 9, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants