Skip to content

Comments

[Feature] Cards bonus Filter#216

Open
Zoma-Ancestral wants to merge 4 commits intomainfrom
feature/card-filter
Open

[Feature] Cards bonus Filter#216
Zoma-Ancestral wants to merge 4 commits intomainfrom
feature/card-filter

Conversation

@Zoma-Ancestral
Copy link
Collaborator

@Zoma-Ancestral Zoma-Ancestral commented Mar 26, 2024

Added a sort and filter function for cards

I copied the code from Breeding for shiny pets and then adapted it for cards.
I noticed some lag on the page with those changes, I'll take any advice to improve performance, maybe I messed up for the useEffect and there is some kind of loop, or maybe it's just that there are a lot of cards

Either through cardifier or with the rift bonus
If no sorting or filter will display as usual, but else will display cards as a list without separating different sets
@vercel
Copy link

vercel bot commented Mar 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
idleon-efficiency ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 26, 2024 3:50pm

Copy link
Owner

@Sludging Sludging left a comment

Choose a reason for hiding this comment

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

Overall layout and feature looks good to me but I think we need to do something about the filters that is a bit better .. it's pretty messy right now 🤔

Let me know if you have ideas .. otherwise we can maybe release as is (once we fix the loading filter issues) and iterate on it overtime based on feedback

@Corbeno
Copy link
Collaborator

Corbeno commented Mar 29, 2024

I wouldn't suggest merging this just yet. The useEffects are setup in a way that causes infinite looping, which is over the entire card metadata list (250+ objects). Slows down the whole tab a ton.

I tried to fix it myself, but the filtering is pretty messy. I would suggest changing the storage of the card data so it can be filtered easier on the fronted. It's not a good idea to have this much logic in the frontend files.

I love the feature though! Very good UI!

@Sludging
Copy link
Owner

Sludging commented Mar 31, 2024

I wouldn't suggest merging this just yet. The useEffects are setup in a way that causes infinite looping, which is over the entire card metadata list (250+ objects). Slows down the whole tab a ton.

Well infinite is not a good choice of words haha, since it actually loads fine.

Also can you clarify which portion has a useEffect that changes something it depends on? I couldn't see anything that would cause a loop but maybe I missed it 🤔

@Corbeno

setCardsData(theData.get("cards"));
cardSets.forEach(cardSet => { cardSet.cards = (cards) ? cards.filter(card => card.data.category == cardSet.cardSetName) : [] });

const filterOptions = cards.filter(card => card.displayName != "New Monster?").map(card => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for taking so long to respond.

I think it's here. If you debug it, it will always keep breaking in this useEffect() even after the page has been rendered for a while. I think it's because you are depending on cardSets but also making changes to it on this line.

Copy link
Owner

Choose a reason for hiding this comment

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

Removing the cardSets from the useEffect dependancy array solves the constant re-render problem, and we don't need to depend on it so that's fine.

However, I am finally noticing what you guys mean by the page being "laggy" or "slow" and I still haven't found a way to improve it .. maybe there's little we can do with so much information being rendered .. not sure.

I tried moving a lot of the calculations to the "pre-calculation" stage but that made little difference.

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