grafana / scenes

Build Grafana dashboards directly in your Grafana app plugins.
https://grafana.com/developers/scenes
Apache License 2.0
133 stars 20 forks source link

UrlSync: Add browser history step when updating URL #760

Closed torkelo closed 1 month ago

torkelo commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

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 commented 1 month ago

closing in favor of https://github.com/grafana/scenes/pull/878