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

Dashboard Improvements I #3608

Merged
merged 22 commits into from
Mar 15, 2021
Merged

Dashboard Improvements I #3608

merged 22 commits into from
Mar 15, 2021

Conversation

paolodamico
Copy link
Contributor

@paolodamico paolodamico commented Mar 9, 2021

Changes

This is the fist PR in the series outlined in #3344 & #3551 to significantly improve our dashboards experience. Addresses some of the issues/improvements outlined in #3547.

  • Fully refactors the action items of dashboards to simplify use and have more clarity around sharing.

  • Introduces the concept of "Edit mode" for dashboards where they can be moved, resized, edited, etc. instead of the locking mechanism that we had before. This relies on the fact that by default most of the time we look at dashboards, we're not editing them. It also makes it more clear when you're in this mode and the fact that you can move things around (dashboards will bounce too). As a side note: edit mode can be accessed on mobile devices by long pressing an item.

Before

After

  • Adds an indicator when dashboards are shared.

Screenshot 2021-03-09 at 12 14 20 PM

- Adds data instrumentation for a bunch of dashboard-related events that we didn't have before.

In addition to the above, this PR also:

  • Significantly simplifies the logic of how dashboards are shown (loading states, error states, …)
  • Fixes a bug in which when a non-existent dashboard was opened, we always showed it as an empty dashboard instead of non-existent (leading to further weirdness when trying to add stuff)
  • Removes individual dashboard item refreshes in favor of only global dashboard refresh to simplify UX.
  • Fixes the UI of toasts in general.

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-dashboard-impro-vjawyc March 9, 2021 17:13 Inactive
@timgl timgl temporarily deployed to posthog-dashboard-impro-vjawyc March 9, 2021 17:44 Inactive
@paolodamico
Copy link
Contributor Author

paolodamico commented Mar 9, 2021

Help needed (maybe @macobo as I believe you implemented this), when calling refreshAllDashboardItems we update each item in their own scope, which means that the general items object in the upper scope logic (dashboardLogic.js) doesn't get updated. Not sure if we should propagate the change upwards, re-fetch everything, or just do a general refresh. The UX problem this is causing is that the "Last refreshed" label now introduced in the dashboards gets outdated because the items don't get updated. Thoughts?

EDIT: Opening an issue as I've seem to identified another problem.

@paolodamico paolodamico mentioned this pull request Mar 9, 2021
2 tasks
@paolodamico paolodamico added feature/dashboards Feature Tag: Dashboards UI/UX labels Mar 9, 2021
@timgl timgl temporarily deployed to posthog-dashboard-impro-vjawyc March 9, 2021 20:28 Inactive
@timgl timgl temporarily deployed to posthog-dashboard-impro-vjawyc March 10, 2021 00:51 Inactive
@paolodamico paolodamico marked this pull request as ready for review March 10, 2021 00:57
@corywatilo
Copy link
Contributor

My only suggestion would be to move the Shared icon(s) to the right side of the menu.

Related: What happens if the dashboard name is long? Seems like it would look cleaner to truncate the name with an ellipsis rather than wrap, along with a tooltip in case dashboard names are namespaced to where you can't make out the difference between them). Example: "Marketing > Q1 Initiativ..."

image

@timgl timgl temporarily deployed to posthog-dashboard-impro-vjawyc March 10, 2021 15:03 Inactive
@paolodamico
Copy link
Contributor Author

Thanks for the review! All feedback addressed, let me know if we're good to merge. FYI here's how handling will now look like for super long dashboard names.

@paolodamico paolodamico mentioned this pull request Mar 10, 2021
4 tasks
Copy link
Contributor

@yakkomajuri yakkomajuri left a comment

Choose a reason for hiding this comment

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

code-wise, a few nits from my side otherwise looks good!

non-blockers really, up to you how to go about it

frontend/src/scenes/dashboard/Dashboard.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/dashboard/DashboardItems.tsx Outdated Show resolved Hide resolved
if (isOnEditMode) {
clearDOMTextSelection()
window.setTimeout(clearDOMTextSelection, 200)
window.setTimeout(clearDOMTextSelection, 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a cleaner way to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there is, tbh I'm not entirely sure why we're doing this. Though, this PR is not changing the behavior (copied over). To move forward I would love if someone with more context on this could help us determine how to improve it.

@paolodamico paolodamico merged commit b27dace into master Mar 15, 2021
@paolodamico paolodamico deleted the dashboard-improvements-I branch March 15, 2021 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/dashboards Feature Tag: Dashboards
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants