Skip to content

fix:issue-986 #1028

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

Merged
merged 1 commit into from
Jan 11, 2021
Merged

Conversation

yoution
Copy link
Contributor

@yoution yoution commented Jan 9, 2021

Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

@yoution the current fix has some issues.

If I create a challenge or update the challenge and go the view details page without page reloading the groups would not be shown, see demo video https://monosnap.com/file/oAMpt31iiCQ5mjHoLEE0DprDt2a8vb

This happens, because data is stored inside challengeDetails which is updated during challenge update. Please, don't store data there, as we would always lost it.

The best solution IMHO would be not to store groups in Redux store, and just create a hook, which would return group names by group ids, like const goupsDetails = useGroupDetails(groupIds = [123, 435, 3453]). Though it could be a little bit tricky to avoid unnecessary re-renders.

So storing in Redux store is also fine, but then we have to choose some new place for groups data which is not rewrtitten by other actions.

@maxceem
Copy link
Contributor

maxceem commented Jan 11, 2021

@yoution also, I've noticed that file src/actions/challenges.js has many unnecessary formatting changes. Please, don't make automatic formatted changes as we are doing several fixes in parallel, it could lead to the merge conflicts later. Update only the code which has to be updated for the issue.

@yoution
Copy link
Contributor Author

yoution commented Jan 11, 2021

@maxceem please review

Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Thanks, @yoution, works good now.

@maxceem maxceem merged commit 6597d74 into topcoder-platform:cf-jan-2021 Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants