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.

Sign up for GitHub

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

Jump to bottom

Add loader for profile cards #515

Merged
merged 4 commits into from
May 4, 2022
Merged

Add loader for profile cards #515

merged 4 commits into from
May 4, 2022

Conversation

nenadV91
Copy link
Contributor

@nenadV91 nenadV91 commented May 4, 2022

Summary

Fixes #492

  • adds a circle spinner while the profile cards balances are loading
  • also removes the loader in any case after 5 sec

To test

  • go to profile page
  • test with bot connected and not connected accounts
  • there should be a circle spinner while the card balances are loading

@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2022

CLA Assistant Lite All Contributors have signed the CLA.

@nenadV91 nenadV91 requested review from a team May 4, 2022 13:04
@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2022

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

alfetopito
alfetopito reviewed May 4, 2022
View reviewed changes
src/custom/pages/Profile/index.tsx Outdated Show resolved Hide resolved
src/custom/pages/Profile/index.tsx Outdated Show resolved Hide resolved
src/custom/pages/Profile/index.tsx
const isCardsLoading = useMemo(() => {
let output = isVCowLoading || isLockedLoading || !library

// remove loader after 5 sec in any case
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is that? If it's still loading why not show the loader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think after 5 sec if its still in loading state there is probably some bug and we can show cards instead. I didn't see this happen but I would add this just in case

elena-zh
elena-zh approved these changes May 4, 2022
View reviewed changes
Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

LGTM

@nenadV91
Copy link
Contributor Author

nenadV91 commented May 4, 2022

@elena-zh I've changed the implementation a bit, if you could please take another quick look 🙏

@fairlighteth
Copy link
Contributor

Only comment here is to use the theme.text1 or theme.white for the loader color. Initially I thought it's nicer to show the cards up front while they are 'loading' with this shimmer effect. But given we don't know what cards to show (vCOW? LGNO?) the page would either way 'jump' a bit and thus would be personally OK with your implementation.

@nenadV91 nenadV91 requested a review from alfetopito May 4, 2022 14:39
@elena-zh
Copy link

elena-zh commented May 4, 2022

@nenadV91 , LGTM!

@nenadV91
Copy link
Contributor Author

nenadV91 commented May 4, 2022

@fairlighteth I've updated the spinner

@nenadV91 nenadV91 merged commit 97c1d58 into release/1.14 May 4, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 4, 2022
@alfetopito alfetopito deleted the 492/profile-cards-loading branch May 5, 2022 08:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@elena-zh elena-zh elena-zh approved these changes

@alfetopito alfetopito alfetopito approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nenadV91 @fairlighteth @elena-zh @alfetopito