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

Make saved insights meta fields editable in-line #6159

Merged
merged 14 commits into from
Oct 5, 2021

Conversation

alexkim205
Copy link
Contributor

@alexkim205 alexkim205 commented Sep 28, 2021

Changes

Closes #6131.

Should've gotten most of the edge cases with different types of inputs but please feel free to poke around and break things.

Demo

Screen.Recording.2021-10-04.at.12.45.03.AM.mov

How did you test this code?

  • tests for new logics
  • tests for new utils

@alexkim205 alexkim205 self-assigned this Sep 28, 2021
@timgl timgl temporarily deployed to posthog-pr-6159 September 28, 2021 22:22 Inactive
@timgl timgl temporarily deployed to posthog-pr-6159 September 29, 2021 20:41 Inactive
@timgl timgl temporarily deployed to posthog-pr-6159 September 30, 2021 19:52 Inactive
@timgl timgl temporarily deployed to posthog-pr-6159 October 1, 2021 01:29 Inactive
@timgl timgl temporarily deployed to posthog-pr-6159 October 4, 2021 03:33 Inactive
@timgl timgl temporarily deployed to posthog-pr-6159 October 4, 2021 07:29 Inactive
@timgl timgl temporarily deployed to posthog-pr-6159 October 4, 2021 07:34 Inactive
@timgl timgl temporarily deployed to posthog-pr-6159 October 4, 2021 07:49 Inactive
@alexkim205 alexkim205 requested review from clarkus, mariusandra, rcmarron and pauldambra and removed request for rcmarron October 4, 2021 07:49
@alexkim205 alexkim205 marked this pull request as ready for review October 4, 2021 07:50
@alexkim205 alexkim205 changed the title Updates to saved insights meta fields Make saved insights meta fields editable in-line Oct 4, 2021
Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

Looks good to me 🙌 . No reason to block this merge on the idea of extracting a component from the editable text although (if nothing else) it does save the future reader comparing the title and description to understand differences

frontend/src/scenes/insights/insightLogic.tsx Outdated Show resolved Hide resolved
clarkus
clarkus previously requested changes Oct 4, 2021
Copy link
Contributor

@clarkus clarkus left a comment

Choose a reason for hiding this comment

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

I like this change. Based on the feedback during the demo today, we might consider making renaming, description editing, tagging available outside of the edit view. Is it technically feasible to save those single-attribute changes outside the edit workflow? Great work, Alex!

@alexkim205
Copy link
Contributor Author

Thanks for the feedback everyone!

Is it technically feasible to save those single-attribute changes outside the edit workflow?

Yup it should be. The only difference would be if the user clicks "Done" outside of the edit workflow, it will permanently save the metadata for that insight. This is different from behavior in the edit workflow, where clicking "Done" will just save the state of the metadata and clicking "Save" will permanently save the final state.

@alexkim205
Copy link
Contributor Author

Added edit functionality to view modes and cleaned up FF's for this feature.

Screen.Recording.2021-10-04.at.4.19.09.PM.mov

@alexkim205 alexkim205 dismissed clarkus’s stale review October 4, 2021 23:22

addressed feedback

@timgl timgl temporarily deployed to posthog-pr-6159 October 4, 2021 23:22 Inactive
@timgl timgl temporarily deployed to posthog-pr-6159 October 4, 2021 23:50 Inactive
@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@25163d0). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6159   +/-   ##
=========================================
  Coverage          ?   91.77%           
=========================================
  Files             ?      391           
  Lines             ?    29584           
  Branches          ?     2510           
=========================================
  Hits              ?    27150           
  Misses            ?     1906           
  Partials          ?      528           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25163d0...8300128. Read the comment docs.

@mariusandra
Copy link
Collaborator

mariusandra commented Oct 5, 2021

Overall LGTM. Going to merge since I want to work on the insight logics a bit today, and best to have it in... and it's feature flagged.

The twothree UX issues I found here are:

  1. Editing tags inside the "view" mode doesn't save them
  2. When I reload the insight, sometimes I see the edited text, sometimes not.
  3. Sometimes I get to a state where it creates a new insights on every reload

All these are proper blockers to unflagging saved insights, but not for merging this PR.

@mariusandra mariusandra merged commit c2a1f30 into master Oct 5, 2021
@mariusandra mariusandra deleted the feat/new-saved-insights-metadata branch October 5, 2021 10:56
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.

Updates to saved insights meta fields
6 participants