-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Bugfix][State Management] 'state:storeInSessionStorage' doesn’t take effect for dashboard without full page reload #56284
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
Conversation
…rd without full page reload
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
…thout full page reload”
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
majagrubic
left a comment
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.
Looks good!
flash1293
left a comment
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.
Code LGTM, didn't test
| stateManagementConfigProvider.disable(); | ||
| }); | ||
|
|
||
| app.factory('history', () => createHashHistory()); |
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.
nit: It's not a big deal, but as we are trying to get away from angular we could already do that step here and call createKbnUrlStorage directly within the controller instead of letting angular handle this. Not blocking this merge, we can also tackle that later.
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.
I needed to make sure that Listing page and Dashboard page are using the same instance of kbnUrlStateStorage.
When we clean up angular from here, these 2 just should move back somewhere to the root of the plugin and then passed down to react components.
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.
Why is this required? I'm asking because the dashboard listing page and the actual dashboard page are two separate apps - if you are switching between the two it is unmounting and re-mounting the application anyway (new angular context and so on). https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/kibana/public/dashboard/plugin.ts#L128
That was necessary because of the different urls - the listing page is the dashboards app, the individual dashboard is the dashboard app.
… effect for dashboard without full page reload (elastic#56284) * bugfix: 'state:storeInSessionStorage' doesn’t take effect for dashboard without full page reload * add test “changing 'state:storeInSessionStorage' also takes effect without full page reload”
Summary
This fixes a regression noticed by @majagrubic when working on #55443.
The regression caused by: #55158
The bug
'state:storeInSessionStorage' doesn’t take effect for dashboard without full page reload
To reproduce
It starts working after page reload.
The fix
kbnUrlStateStorageis responsible for updating the state in the url. The problem is, that it was initialised with uiSettings('state:storeInSessionStorage') only once! Together with angular modules initialisation. -kibana/src/legacy/core_plugins/kibana/public/dashboard/np_ready/application.ts
Line 73 in d010abc
I had to move the initialisation inside angular so each time angular app gets mounted, it creates new instance with proper value from uiSettings.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers