UrlSync: Variable changes adds browser history steps #882
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ifrost
reviewed
Aug 29, 2024
| @@ -428,6 +446,16 @@ export class MultiValueUrlSyncHandler<TState extends MultiValueVariableState = M | |||
| this._sceneObject.changeValueTo(urlValue); | |||
| } | |||
| } | |||
|
|
|||
| public performBrowserHistoryAction(callback: () => void) { | |||
There was a problem hiding this comment.
Nice! 🎉 Do you think it could be the default behavior of SceneObjectUrlSyncConfig and AdHocFiltersVariableUrlSyncHandler
There was a problem hiding this comment.
yea, probably. pushed an update to #878 that adds it to SceneObjectUrlSyncConfig
adrapereira
approved these changes
Feb 13, 2025
| ); | ||
| }} | ||
| onChange={(newValue, action) => { | ||
| if (action.action === 'clear' && noValueOnClear) { | ||
| model.changeValueTo([]); | ||
| model.changeValueTo([], undefined, true); |
There was a problem hiding this comment.
Would it make sense to add an extra method to avoid repeatedly calling the changeValueTo method with undefined and true?
For example:
- variable.changeValueTo(value, undefined, true);
+ variable.changeValuePushToHistory(value);
+
+ public changeValuePushToHistory(value: VariableValue) {
+ this.changeValueTo(value, undefined, true);
+ }It's just some syntactic sugar, but will make it easier to read in the plugin code. Alternatively each plugin can also implement their own util to handle this changeVariablePushToHistory(variable, value) if we don't want to add it to the library.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This works on top of #878
and explores adding browser history steps for variables.
Works surprisingly well. The challenge here is that not all variable value changes should create URL history steps.
Given a valid state of datacenter=US , cluster=US-1
To solve this we need to differentiate between variable changes that are due to user actions and those that come due to parent variable change. which is why I am adding isUserAction bool to changeValueTo
works surprisingly well