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 item description (1/2) #4240

Closed
wants to merge 21 commits into from
Closed

Conversation

liyiy
Copy link
Contributor

@liyiy liyiy commented May 6, 2021

Changes

(1/2) Updates for dashboard item editing on #3551

Screen Shot 2021-05-06 at 3 06 17 PM

  • Follow up PR (2/2) will include
    • allowing dashboard item title to be edited
    • some more style fixes
    • e2es

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious

@timgl timgl temporarily deployed to posthog-pr-4240 May 6, 2021 19:17 Inactive
@liyiy liyiy force-pushed the dashboard-item-description branch from 41dd956 to 0083f2a Compare May 6, 2021 23:04
@liyiy liyiy marked this pull request as ready for review May 6, 2021 23:04
@liyiy liyiy changed the title Dashboard item description Dashboard item description (1/2) May 6, 2021
@liyiy
Copy link
Contributor Author

liyiy commented May 6, 2021

There's a lot of the same code between the dashboard description edit and dashboard item description edit, maybe worth going over at a later time to see which parts to consolidate ^_^

@liyiy
Copy link
Contributor Author

liyiy commented May 6, 2021

confused with prettier failure that doesn't show what the error is in the Dashboard.scss file 😦

@liyiy liyiy requested review from paolodamico and macobo May 6, 2021 23:37
Copy link
Contributor

@paolodamico paolodamico left a comment

Choose a reason for hiding this comment

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

Looking pretty good!! Reviewed this from a UX perspective,

  1. I think we should keep the same consistent behavior for editing as we do for dashboards (see below). Clicking to edit instead of the button, close if you click outside, cmd/cntrl + enter to submit.
  2. I think dashboard item might be confusing. Maybe dashboard insight?
  3. Related to the above the placeholder text should be: "Add a description for this dashboard insight..."
  4. The description should not be muted if it's set (show in default text color). Probably worth having it the same font size as the rest (unsure, let's try it out).
  5. You should be able to edit the item's description from the dashboard page.

frontend/src/models/dashboardItemsModel.tsx Outdated Show resolved Hide resolved
@timgl timgl temporarily deployed to posthog-pr-4240 May 7, 2021 02:52 Inactive
@timgl timgl temporarily deployed to posthog-pr-4240 May 7, 2021 20:28 Inactive
@timgl timgl temporarily deployed to posthog-pr-4240 May 7, 2021 20:48 Inactive
@timgl timgl temporarily deployed to posthog-pr-4240 May 8, 2021 18:20 Inactive
@timgl timgl temporarily deployed to posthog-pr-4240 May 10, 2021 13:55 Inactive
@liyiy
Copy link
Contributor Author

liyiy commented May 10, 2021

@macobo I addressed most of the concerns on @paolodamico's comment but in favor of keeping this PR from spiraling, I will address everything else in the second PR of this (editing dashboard insight description on the dashboards page)

@liyiy liyiy requested a review from macobo May 10, 2021 13:57
@liyiy liyiy closed this May 10, 2021
@paolodamico paolodamico deleted the dashboard-item-description branch November 23, 2021 17:12
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.

4 participants