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

UrlSync: Add browser history step when updating URL #760

Closed
wants to merge 6 commits into from

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented May 30, 2024

Previously when the UrlSyncManager updates the URL due to scene object state change this was done with a "replace: true", meaning no browser history state was created.

For things time range change or variable filters this the old architecture did create new browser steps here, the main reason I figured a browser history step is not expected is that the user did not click on a link so so not sure adding/removing UI filters should create browser history step but think this was wrong, we should align to how the old architecture works and always create a new browser history step here. Did a few tests with other filter UIs (GitHub PR list) and they do create new browser steps when adding / removing filters.

The only awkward scenarios is when some state is synced to URL that has no immediate visible change, like SceneRefreshPicker auto refresh property, this is synced to URL so changing this will now also create a browser history step. But so far that is the only awkward side effect.

Would be very happy for feedback on this one, alternate ideas, should there be an option (global, per scene object, etc)

@ivanortegaalba
Copy link
Collaborator

I think it makes sense to add a history push, but I'd also like to be able to skip it if needed.

In a micro frontend arch I used to work in the past, we had an API like this:

this.setState({propThatGoesIntoURL: value}, {replaceHistory: true})

And by default, it is false. So when you set state into a component that has state in sync with the URL state, you can skip that story entry.

@darrenjaneczek
Copy link
Contributor

Related to "Data trails," this may have some impact there.
If urlStates change with browser back/forward activity, it may trigger the creation of new history nodes.
It may be necessary to store the 'currentStep' as part of the URL history step somehow, so that we can detect that and act accordingly.

Given this change in philosophy, do you think it is appropriate for history back/forward activity to traverse the steps of data trails?

@dprokop
Copy link
Member

dprokop commented Jun 3, 2024

Agree with @ivanortegaalba comment here. I think it's fine to have the default push but it may be useful to allow opting out from creating a history entry.

Think one of the most unintuitive history push in the old arch is the time range change (i know this is bringing it back), but overall i find it quite confusing that there's a query executed when i want to navigate back, especially if i tinkered with the time range picker multiple times. To me, interacting with UI elements that do not semantically affect queries (like filters or group by) should not create new history entries.

@torkelo
Copy link
Member Author

torkelo commented Jun 3, 2024

To me, interacting with UI elements that do not semantically affect queries (like filters or group by) should not create new history entries.

@dprokop do you feel the same for GitHub issues / PR filters? (they do update URL and create new browser history states). I think kibana also creates browser history states when you change filters or time range, not sure what data dog does. I am also a bit torn.

I don't know how to cater to both here, I think it's either or. Maybe we can have an option per scene object (or part of urlSync object / interface).

@ifrost
Copy link

ifrost commented Aug 12, 2024

Are you planning to move forward with this idea? If the plan is not to do it - do you have any guidelines on the best way to add it to an app plugin (i.e. push the history on variable changes or any changes synced to the URL)?

I think kibana also creates browser history states when you change filters or time range, not sure what data dog does. I am also a bit torn.

BTW. In Explore every change to the URL is pushed to the history and I think it's quite handy.

@gfonseca-tc
Copy link

Being able to move back and forward in time ranges is a major blocker for us to move to the new Scenes dashboards. It makes it really hard to troubleshoot anything if you can't return to the previous time range you were easily. +35 to this feature!

@torkelo
Copy link
Member Author

torkelo commented Aug 20, 2024

One big challenge here is how chain variables updates (1 by one).

Let's say you have 3 variables (datacenter => cluster => job) where cluster depends on datacenter, job depends on cluster etc.

And you change datacenter in the UI, which automatically causes cluster to change, then job changes automatically as they are chained. You hit browser back and you get back to the previous value for job (which does not exist for the selected data center and cluster).

@gfonseca-tc
Copy link

I get what you mean, seem complicated to handle, but I was thinking in a simpler use case, just for time ranges for instance. The following steps, imo, should work as the previous UI.

  • Open a dashboard
  • Change time range
  • Don't do any further change, just scroll the dash
  • Hit the back button and get to previous time range.

Currently in scenes the last step gets you to a page that don ´t make any sense with the navigation you have been doing.

@torkelo
Copy link
Member Author

torkelo commented Aug 28, 2024

closing in favor of #878

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.

6 participants