Skip to content

Conversation

@minimalsm
Copy link
Contributor

Description

  • In the spirit of client diversity I refactored the naive random short we were using to a lodash shuffle instead.
  • Also separated the declaration of the client data from the sorting logic

Related Issue

None

@corwintines
Copy link
Member

I like the change when it comes to sorting. I just feel like the whole useState and useEffect is a bit overkill for this if we just sort a list one time. Could we remove these and simplify this component? Curious what you think @minimalism

@minimalsm
Copy link
Contributor Author

@corwintines sounds good but i'm not 100% clear on the tradeoffs here. maybe worth asking someone else to weigh in?

Only downside I see of doing it this way is that the cards will be reordered after the page renders but the current implementation has layout shift anyway so both solutions are a bit jarring 🤷‍♂️.

Copy link
Member

@corwintines corwintines left a comment

Choose a reason for hiding this comment

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

Actually after looking further, useEffect would be a proper lifecycle method for this. LGTM!

@corwintines
Copy link
Member

@minimalsm just have some merge conflicts, but otherwise good for me.

@wackerow
Copy link
Member

🎉 Nice job!

@wackerow wackerow merged commit 179bbdc into dev Dec 10, 2021
@wackerow wackerow deleted the refactorEth2ClientList branch December 10, 2021 05:01
@wackerow wackerow mentioned this pull request Dec 10, 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.

4 participants