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

feat: Notebooks History #17644

Merged
merged 43 commits into from
Sep 28, 2023
Merged

feat: Notebooks History #17644

merged 43 commits into from
Sep 28, 2023

Conversation

benjackwhite
Copy link
Contributor

@benjackwhite benjackwhite commented Sep 27, 2023

Problem

We autosave changes which is great but sometimes you might make a change and think "whoops - I want to revert".
Undo redo works whilst the Notebook is open but after that you might want to revert to a previous version.

Changes

  • Adds a popout to the sidebar for the "history" using the Activity API.
  • Selecting an option previews it, which copies the old content out and saves forward.
  • Fixes scrolling to a node, to only do it when the settings are opened.

2023-09-28 10 28 25

TODO

  • Actually implement the revert
  • Fix the UI / styling
  • Maybe preview the change?

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

7 snapshot changes in total. 0 added, 7 modified, 0 deleted:

  • chromium: 0 added, 7 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

7 snapshot changes in total. 0 added, 7 modified, 0 deleted:

  • chromium: 0 added, 7 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

7 snapshot changes in total. 0 added, 7 modified, 0 deleted:

  • chromium: 0 added, 7 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

7 snapshot changes in total. 0 added, 7 modified, 0 deleted:

  • chromium: 0 added, 7 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@benjackwhite benjackwhite marked this pull request as ready for review September 28, 2023 08:44
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.

This is awesome... a few nitpicks but otherwise 🚢

If there's only a single version and I open the history, then I can click on it and am offered the opportunity to revert. Really the current version shouldn't offer that option


My first instinct to close the history was to repeat the action I'd done to open it...

2023-09-28 10 17 27


For 100% score on the PR it'd be awesome if the activity log distinguished between a revert and a not-revert

Screenshot 2023-09-28 at 10 18 06

But that's very finicky of me

@benjackwhite
Copy link
Contributor Author

If there's only a single version and I open the history, then I can click on it and am offered the opportunity to revert. Really the current version shouldn't offer that option

That's a good point! Modified it to show the "created this" state for the very first activity and mark the "current" version as current so that you can't revert to it.

My first instinct to close the history was to repeat the action I'd done to open it...

It is now a toggle

For 100% score on the PR it'd be awesome if the activity log distinguished between a revert and a not-revert

Yeah I thought about this. The tricky bit is it is more of a "revert commit" so there's no current way of distinguishing it.
We could perhaps add some meta fields to the content but I want to avoid making a schema decision here. Good idea for follow up though.

If we had a field to say something like "version this was copied from" that could be stored somewhere, at least in the activity log maybe 🤔

@benjackwhite benjackwhite merged commit 78dc992 into master Sep 28, 2023
@benjackwhite benjackwhite deleted the feat/notebooks-history branch September 28, 2023 12:48
@pauldambra
Copy link
Member

Ah, I was too slow :D

A couple of nits, but not blocking so 🤷

  • when you create a notebook from a previous version the toast says you created it from a template, but you didn't
  • if you have the history panel open and edit the notebook, it doesn't automatically reload, so you have to close and re-open it to see the current history

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.

3 participants