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

Replace url instead of pushing when changing properties/filters #4966

Merged
merged 2 commits into from
Jul 2, 2021

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Jul 1, 2021

Changes

  • Makes it so much better to navigate around insights!
  • Instead of pushing a new url whenever filters change, we now replace the url instead.
  • In short: normal back/forward navigation between tabs and any changes to filters inside a tab won't change the back button.
  • Side-effect: the bug from going to persons to a person is now fixed, as are many others like Back button doesn't work as expected: Cohorts -> Sessions #4748
  • Upgrades kea-router to 1.0.0 that supports returning { replace: true } from actionToUrl

2021-07-02 00 35 48

2021-07-02 00 52 30

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
  • Frontend/CSS is usable at 320px (iPhone SE) and decent at 360px (most phones)
  • Breaking changes are backwards-compatible. Ensure old/new frontend requests work with new/old backends, and vice versa.

@timgl timgl temporarily deployed to posthog-pr-4966 July 1, 2021 22:41 Inactive
@mariusandra mariusandra temporarily deployed to posthog-pr-4966 July 1, 2021 23:07 Inactive
setFilters: () => buildURL(),
loadSessions: () => buildURL(),
setFilters: () => buildURL({}, true),
loadSessions: () => buildURL({}, true),
setSessionRecordingId: () => buildURL(),
closeSessionPlayer: () => buildURL({ sessionRecordingId: undefined }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why did you chose to make an exception here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean the last two? They basically go from the list to a individual recording, so that's basically going to a separate page that we could go back from.

@alexkim205
Copy link
Contributor

lgtm!

@mariusandra mariusandra merged commit 7bcd721 into master Jul 2, 2021
@mariusandra mariusandra deleted the properties-replace-url branch July 2, 2021 15:02
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