-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Infra UI] Use alternative query string serialization function #29361
[Infra UI] Use alternative query string serialization function #29361
Conversation
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me this doesn't implement the desired behavior as described in #24028 . Instead, the back button jumps back to /app/kibana#/home?_g=()
. Is this intentional?
I also see the same behaviour. |
True, it doesn't represent every small state change in the history. It merely fixes the bug that navigating "one step back" via the browser chrome or the The code was written to track navigation between apps or sections of apps (i.e. other kibana apps, the waffle map, the node details pages and the logs). I'd be interested to hear what the arguments for a different navigation paradigm would be. |
Maybe we should narrow the original bug report to the "navigating back does not have any effect" aspect and split off a separate "what state changes should be represented in the browser history" discussion? |
As this bug fix is aimed at Deferring discussion from the meeting: Do we want Or Do we want Originally with the review I was (as I think Sonja was) basing my comment off of the comment in #24028 and testing that as the expected behaviour. As for what I'd deem expected behaviour, I think what we have now is good. If there's one thing we don't want to do - in my previous experience - is to try and mix and match the two (i.e. sometimes query parameters cause a new history entry, sometimes they don't). We currently have The ultimate goal is that a URL can be used to 100% replicate what a user was looking at, and that it can be shared with others, that we have. There is one area where I'm torn though, and that's pagination (in general, not even just aimed at our infra codebase). Pagination parameters end up in |
Good points, @Kerry350, thank you. So maybe we actually do want to mix-and-match for some cases like pagination (to be decided once that use cases arises). Just for reference, the pagination state of most tables in Kibana is not represented in the URL. |
Yeah — to be honest we probably do. "If there's one thing we don't want to do" was a bit hasty, the previous experience I speak of was manually adding query parameter route change support into an old version of the Backbone library router, and then getting that to synchronise nicely with duplicated Redux state. It was a nightmare, but it is not reflective of the landscape these days 😄
👍 Cool, another thing to bear in mind then, best to keep consistency throughout the app. Ultimately, I think if we did want to add support for the query parameters, and also make it so it had the power to be mix and match, if we sat down and planned out an API as a team we could come up with something really nice (only if the need ever arises). Also, two extra things:
|
Thanks for the write up @Kerry350. I agree with your comment, so this PR LGTM as well. |
Thanks for the input, everyone!
That's a good general guideline, but there's lots of unfortunate, historically grown behaviour in Kibana that we would probably do well to ignore. 🕸️ In this case maybe the lesson from Kibana would be to use the URL sparingly for state that really needs to be bookmarkable/sharable. |
Great job getting this fixed @weltenwort ! |
This replaces the standard
queryString.encode
/decode
functions with Kibana's specialized querystring serialization functions which seem to work around angularjs$router
quirks.As a result, most local state changes in the Infrastructure UI no longer lead to an ever-expanding list sequence of history states. This restores the forward- and backward-button navigation between sites of the app.
fixes #24028