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

Add pinned dashboards to new navigation #2906

Merged
merged 13 commits into from
Jan 13, 2021

Conversation

paolodamico
Copy link
Contributor

@paolodamico paolodamico commented Jan 10, 2021

Changes

  • Adds an easily accessible list of dashboards (with a special section for pinned dashboards), that appears when hovering the dashboards menu item.
  • Handles the special case where no dashboards have been created.
  • If dashboards have been created but there are none for either the non-pinned or pinned sections, the sections will not be shown.

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-navigation-1775-pimqln January 10, 2021 21:08 Inactive
@Twixes
Copy link
Collaborator

Twixes commented Jan 10, 2021

My 2c: "Pinned dashboards" shouldn't appear if there are none. Pinnability should be discoverable without the "please pin something" prompt, as that takes quite a bit of space.

@paolodamico
Copy link
Contributor Author

My 2c: "Pinned dashboards" shouldn't appear if there are none. Pinnability should be discoverable without the "please pin something" prompt, as that takes quite a bit of space.

I have mixed feelings about this. One the one hand I agree with you, it can be a waste of space to have that there (not a great UX). On the other I think getting users to pin dashboards could be pretty powerful. We know that a large percentage of long-term retained users are the ones who create custom dashboards (and presumably look at them regularly), and it can be beneficial in the long run to get more users to pin dashboards. Happy to welcome more feedback on this! Otherwise I think a good approach would be to follow your suggestion, and after this experiment is over, if it still makes sense, we can do an experiment for guiding users to creating their first pinned dashboard.

@Twixes
Copy link
Collaborator

Twixes commented Jan 10, 2021

My concept is that pinning should be discoverable with the list looking like in /dashboard. An intuitive pin button.

/dashboard

@timgl timgl temporarily deployed to posthog-navigation-1775-pimqln January 11, 2021 00:27 Inactive
@paolodamico
Copy link
Contributor Author

paolodamico commented Jan 11, 2021

Feedback addressed, feel free (if you can) to do a full review @Twixes

@Twixes
Copy link
Collaborator

Twixes commented Jan 11, 2021

Personally I'd really love to see an interactive pin button. That'd make pinning easily discoverable.
Also, I think it'd be a bit more aesthetic if section contents (e.g "Pinned dashboards") were left-aligned along with section name, as opposed to inset, but that's nitpicking.

@mariusandra
Copy link
Collaborator

Some smaller nits:
image

  • The arrow is pointing to "insights" not "dashboards". Probably hard/annoying to fix, so can be as is
  • The blue hover is very light and doesn't provide clear visual feedback when hovering. Some grey background effect might improve the UX
  • I'd make the popup hide after clicking on a dashboard. Otherwise I need to move my mouse to see the first graph
  • Something feels a bit off with the left padding. Either everything should be left-aligned, or then just the text left-aligned with the icons to the left of that. (Just proposing, not sure if it'll actually look good in the end)

Bigger bug:

  • It's broken in mobile mode:
    2021-01-11 12 34 31

Possible improvement:

  • Since I see all dashboards in the popup (not just pinned), it might be cool to be able to pin them from within the popup. It feels a bit unintuitive that I need to click on the "dashboards" main sidebar item just to reach a place where I can pin things.

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Just adding an official review as well...

@paolodamico
Copy link
Contributor Author

Thanks @mariusandra & @Twixes for the feedback, will address today and ping you

@timgl timgl temporarily deployed to posthog-navigation-1775-pimqln January 12, 2021 23:23 Inactive
@paolodamico
Copy link
Contributor Author

Thanks for the suggestion @Twixes and the thorough review @mariusandra! All the comments (except the pinning directly from the overlay, which I'm leaving for a future improvement) have been addressed. Would you mind reviewing once more?

@mariusandra
Copy link
Collaborator

Much better! Although I still find the arrow slightly off-putting. I think it would be better without any arrow:

image

.pinned-dashboards-popover {
    z-index: $z_pinned_dashboards_popup;
    &.ant-popover-placement-right {
        padding-left: 0;
    }
    .ant-popover-arrow {
        display: none;
    }
}

... or then as a standard dropdown component, but then the alignment will be bad as well.

Feel free to make these adjustments if you think they improve things. Approved otherwise!

@timgl timgl temporarily deployed to posthog-navigation-1775-pimqln January 13, 2021 14:42 Inactive
@Twixes Twixes temporarily deployed to posthog-navigation-1775-pimqln January 13, 2021 14:48 Inactive
@Twixes
Copy link
Collaborator

Twixes commented Jan 13, 2021

I really tried to make pinning from the popover work, but Link is unbearable and just takes over the whole of MenuItem when present anywhere… Just can't detect any other events (like icon onClick) inside MenuItem because of it. But overall this looks OK.

@paolodamico paolodamico merged commit 6c31678 into master Jan 13, 2021
@paolodamico paolodamico deleted the navigation-1775-pinned-dashboards branch January 13, 2021 15:18
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.

None yet

4 participants