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

Decouple insights components from URL #4329

Closed
paolodamico opened this issue May 13, 2021 · 4 comments
Closed

Decouple insights components from URL #4329

paolodamico opened this issue May 13, 2021 · 4 comments
Assignees
Labels
enhancement New feature or request tech-debt

Comments

@paolodamico
Copy link
Contributor

paolodamico commented May 13, 2021

Currently we store the state of analytics on the URL. Each individual component (e.g. date range filter, interval filter, ...) fetches the state from the URL and updates it there. By now, we've realized this presents quite a few complications:

  • Backward-compatibility issues with URLs when we change parameter definitions.
  • Code gets quite messy, hard to track down where changes are happening.
    • Some repeated logic across insights.
  • Long URLs that are hard to share.
  • Screws up the intuitive back button of behavior of the browser, as going back will undo the previous action that affected the filters instead of actually going back.

We need to think about how best to handle this, but ideally we would store the state someplace else. We might need to store states in the backend too to create reusable URLs.

Additional context

@mariusandra
Copy link
Collaborator

mariusandra commented Jul 1, 2021

Finally had a chance to make progress towards this. First step, #4966 fixes the final of the 4 points:

  • Screws up the intuitive back button of behavior of the browser, as going back will undo the previous action that affected the filters instead of actually going back.

The other points are still valid:

  • Backward-compatibility issues with URLs when we change parameter definitions.
  • Code gets quite messy, hard to track down where changes are happening.
    • Some repeated logic across insights.
  • Long URLs that are hard to share.

@paolodamico
Copy link
Contributor Author

Is this important to prioritize for next sprint (1.31.0 2/2)?

@mariusandra
Copy link
Collaborator

There are two things still to do with this task:

  1. Change view mode /insights/123?insight=TRENDS&bla=asd&date_from=-7d to /insights/123, only add filters if overridden (e.g. changed date filter)
  2. Put filters into hashParams: /insights/123/edit?insight=TRENDS&bla=asd&date_from=-7d --> /insights/123/edit#q={insight:'TRENDS',bla:'asd',date_from:'-7d'}

Regarding priorities, I think we have worse offenders to fix. This should still get done, and should be in the list, but probably low.

@paolodamico
Copy link
Contributor Author

@mariusandra I believe all your recent work and particularly the huge PR merged today addresses this, so closing. But please reopen if I'm missing something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tech-debt
Projects
None yet
Development

No branches or pull requests

2 participants