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

[SIG-2358] User overview page infinite loop on navigation #671

Merged
merged 5 commits into from Mar 11, 2020

Conversation

janjaap
Copy link
Contributor

@janjaap janjaap commented Mar 11, 2020

User overview page infinite loop on navigation

The Issue

Navigating from the users overview page to a user detail page, after a user's data was posted before that navigation occurred, lead the application into an endless loop, causing errors and freezing the page.

The Problem

The cause was the state that was pushed onto the history stack. Removing it solved the problem. It is, however, unclear why providing a value for a history item's state lead to an endless loop. I looked into issues concerning redux, react-router-dom and connected-react-router, but I couldn't find the problem.

The solution

The reason that a value for a history item's state was needed, was that the filter selection needed to be retained when navigating from the overview page to a detail page and back again.

In order to keep that functionality, the settings module has been changed so that it keeps state in a reducer and passes that state into its child component by using a context provider.

The values for the context are updated by the overview page through a dispatch function that is also provided by the context. This way, all underlying pages in the settings module can make use of the state and update it accordingly.

Copy link
Contributor

@jpoppe jpoppe left a comment

Choose a reason for hiding this comment

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

One minor remark, feel free to ignore ;)

import { SET_USER_FILTERS } from '../constants';

describe('signals/settings/actions', () => {
test('setUserFilters', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason why we use the test alias here instead of it?

@jpoppe jpoppe assigned janjaap and unassigned jpoppe Mar 11, 2020
@janjaap janjaap merged commit fe1ca4f into develop Mar 11, 2020
@janjaap janjaap deleted the SIG-2286_main-menu-notification-interaction branch March 11, 2020 12:10
@janjaap janjaap mentioned this pull request Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants