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

Persist redux state in browser #1178

Merged

Conversation

mengweieric
Copy link
Collaborator

@mengweieric mengweieric commented Oct 21, 2022

Signed-off-by: Eric Wei menwe@amazon.com

Description

Currently, redux state data gets lost on page refresh, which leads to bad user experience. This change is to address this problem.

Issues Resolved

#1177

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Eric Wei <menwe@amazon.com>
@mengweieric mengweieric requested a review from a team as a code owner October 21, 2022 18:56
@mengweieric mengweieric self-assigned this Oct 21, 2022
joshuali925
joshuali925 previously approved these changes Oct 21, 2022
@@ -5,21 +5,45 @@

import { configureStore } from '@reduxjs/toolkit';
import rootReducer from '../reducers';
import storage from 'redux-persist/lib/storage';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this use session storage instead of local storage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should use session storage, the default storage is localStorage

Copy link
Contributor

@pjfitzgibbons pjfitzgibbons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eric, could you please describe in more detail how the user is affected before this change?
What happens if the user navigates away from a page by URL and then back?

@mengweieric
Copy link
Collaborator Author

Eric, could you please describe in more detail how the user is affected before this change? What happens if the user navigates away from a page by URL and then back?

Sure, before this change, if some user actions (for example, directly trigger a saved visualizations url in address bar, or simply just reload the page) trigger a page reload/refresh, all the event/visualization data for each tab in event analytics if there's any will get lost. Navigate to other pages from URL or other user actions within the domain would not cause data loss, which is also not part of the problem this change is trying to address.

@@ -5,21 +5,45 @@

import { configureStore } from '@reduxjs/toolkit';
import rootReducer from '../reducers';
import storage from 'redux-persist/lib/storage';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we considered using url parameters? That way users can share a page with given filters or bookmark a page with commonly used filters for convenience?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting approach. I think that's technically a feature. @mengweieric ?

The specific issue addressed by this Pr is the "lost-form-on-refresh" problem. I agree with this Pr as it stands.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a feature, you may create an issue for a feature request.

pjfitzgibbons
pjfitzgibbons previously approved these changes Oct 24, 2022
@joshuali925
Copy link
Member

linking issue opensearch-project/dashboards-observability#62 this fixes item 3

derek-ho
derek-ho previously approved these changes Oct 25, 2022
Copy link
Collaborator

@derek-ho derek-ho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending session/local storage changes

Signed-off-by: Eric Wei <menwe@amazon.com>
@mengweieric mengweieric merged commit ceac84d into opensearch-project:main Nov 2, 2022
@mengweieric mengweieric deleted the feature/redux-persist branch November 2, 2022 16:56
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 2, 2022
* persist redux state

Signed-off-by: Eric Wei <menwe@amazon.com>

* switch to session storage

Signed-off-by: Eric Wei <menwe@amazon.com>

Signed-off-by: Eric Wei <menwe@amazon.com>
(cherry picked from commit ceac84d)
mengweieric added a commit that referenced this pull request Nov 4, 2022
* persist redux state

Signed-off-by: Eric Wei <menwe@amazon.com>

* switch to session storage

Signed-off-by: Eric Wei <menwe@amazon.com>

Signed-off-by: Eric Wei <menwe@amazon.com>
(cherry picked from commit ceac84d)

Co-authored-by: Eric Wei <menwe@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants