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

GITC-389 Remove duplicates from explore page #9695

Merged
merged 2 commits into from
Nov 23, 2021

Conversation

ksolo
Copy link
Contributor

@ksolo ksolo commented Nov 16, 2021

Description

This is a quick fix for resolving the bug in GITC-389. When in append mode (the mode triggered from scrolling), before adding new grants received from the server, it will check to see if it already exists. A more robust fix can be included in the server, that will be added as a new ticket.

Refers/Fixes

GITC-389

Testing

Tested locally

@ksolo ksolo marked this pull request as ready for review November 18, 2021 15:24
@@ -320,8 +320,13 @@ if (document.getElementById('grants-showcase')) {
if (!append_mode) {
vm.grants = [];
}

const previouslyLoadedIds = vm.grants.map(grant => grant.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider: moving this to the vue model to avoid creating the mapping every time we call fetchGrants()

getGrants.grants.forEach(function(item) {
vm.grants.push(item);
if (!previouslyLoadedIds.includes(item.id)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If previouslyLoadedIds was stored as an object against the viewModel we could make this check O(1) instead of O(n) and avoid the need to map vm.grants into previouslyLoadedIds each call...

Suggested change
if (!previouslyLoadedIds.includes(item.id)) {
if (!vm.previouslyLoadedIds[item.id]) {
vm.previouslyLoadedIds[item.id] = item;

@ksolo ksolo force-pushed the gitc-389/prevent-duplicate-grant-results branch from 82d7c49 to 9b69c1d Compare November 21, 2021 23:38
Copy link
Contributor

@gdixon gdixon left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@gdixon gdixon merged commit 89f50d6 into master Nov 23, 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