Skip to content
This repository has been archived by the owner on Feb 2, 2024. It is now read-only.

Loading improvements #80

Merged
merged 25 commits into from
May 4, 2022
Merged

Loading improvements #80

merged 25 commits into from
May 4, 2022

Conversation

alongoni
Copy link
Contributor

@alongoni alongoni commented May 2, 2022

Summary

Closes #71
image

  1. Open the Transaction details page and open the Batch Viewer. Something like this should appear as a loader:
    https://www.loom.com/share/499aa7cda2f449e0b6acf90ce6460e01

@alongoni alongoni self-assigned this May 2, 2022
@coveralls
Copy link

coveralls commented May 2, 2022

Pull Request Test Coverage Report for Build 2271834893

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 44.267%

Totals Coverage Status
Change from base Build 2271129611: 0.0%
Covered Lines: 2170
Relevant Lines: 4173

💛 - Coveralls

@github-actions
Copy link

github-actions bot commented May 2, 2022

@alongoni alongoni requested review from henrypalacios and ramirotw May 3, 2022 21:04
@alongoni alongoni marked this pull request as ready for review May 3, 2022 21:12
@alongoni alongoni requested a review from alfetopito May 3, 2022 21:18
@ramirotw ramirotw requested a review from elena-zh May 3, 2022 22:07
@ramirotw
Copy link
Contributor

ramirotw commented May 3, 2022

Holy cow!

What about adding it to the User details and also to the Order details?

DancingCowGIF

Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

It's loading too fast to me so I cannot see the animation for very long, so I've tried the storybook instead

Screen.Recording.2022-05-04.at.09.29.46.mov

Is it because of color theme that I cannot notice the effect in the eyes?
I mean, here the eyes are black and I only see the right one bouncing a little

src/components/common/CowLoading.tsx Outdated Show resolved Hide resolved
@elena-zh
Copy link

elena-zh commented May 4, 2022

Hey @alongoni , great job!

Fust a nitpick: I noticed, that some elements are still loading in tables when the 'loading cow' disappears. See the video: https://watch.screencastify.com/v/apygTfJrmAvlDxeCshbC
image

Can we 'expand' this loading effect somehow and show it until all page elements are loaded?
image

Also, maybe we could move the 'loading cow' higher on the page

Thanks!

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

I love the initiative! :)

There's a strange effect when loading. First the loader shows in one position, then it quickly charges, and the it disappears. It creates a strange effect.

I would suggest fixing this logo jumping, and maybe experiment with a delay, so if it loads too quickly we don't even show the loader. Or it appears using a CSS animation so it doesn't do this effects.

@ramirotw
Copy link
Contributor

ramirotw commented May 4, 2022

Fust a nitpick: I noticed, that some elements are still loading in tables when the 'loading cow' disappears. See the video: https://watch.screencastify.com/v/apygTfJrmAvlDxeCshbC image

Can we 'expand' this loading effect somehow and show it until all page elements are loaded?

This @elena-zh implies making other code related changes that weren't part of this ticket, if the intention is to replace or hide somehow the intermediate spinners we should do it throughout the app but that also requires more thinking and it makes sense to do it in another ticket so @alongoni can focus on the style behavior. Wdyt?

@elena-zh
Copy link

elena-zh commented May 4, 2022

@ramirotw , I have added this comment to the #21 (comment) issue

@alfetopito
Copy link
Collaborator

Now I can see the pulsating eyes, nice!

One point, though. The animation expands from top->bottom left->right. Can you make it expand from the center of the image instead?

@ramirotw
Copy link
Contributor

ramirotw commented May 4, 2022

@alongoni could you take a look into the build error?

ERROR in /home/runner/work/explorer/explorer/src/apps/explorer/components/TransactionsTableWidget/index.tsx
ERROR in /home/runner/work/explorer/explorer/src/apps/explorer/components/TransactionsTableWidget/index.tsx(9,1):
TS6133: 'Spinner' is declared but its value is never read.

@henrypalacios
Copy link
Contributor

@alongoni I have put the CowLoading component also in the tab Orders to be consistent with all other views.

Selection_520

In the future it may be possible to use shimming with the shape of the content as we did with the volume graph and use Cow loading only for the first rendering.

@henrypalacios henrypalacios merged commit 2f7ee66 into develop May 4, 2022
@henrypalacios henrypalacios deleted the 71-loading-indicator branch May 4, 2022 22:01
@henrypalacios henrypalacios mentioned this pull request May 4, 2022
@alfetopito
Copy link
Collaborator

@alongoni I have put the CowLoading component also in the tab Orders to be consistent with all other views.

Selection_520

In the future it may be possible to use shimming with the shape of the content as we did with the volume graph and use Cow loading only for the first rendering.

I like this idea, could you create an issue for that so we don't forget to get back to it?

@alongoni
Copy link
Contributor Author

alongoni commented May 5, 2022

I love the initiative! :)

There's a strange effect when loading. First the loader shows in one position, then it quickly charges, and the it disappears. It creates a strange effect.

I would suggest fixing this logo jumping, and maybe experiment with a delay, so if it loads too quickly we don't even show the loader. Or it appears using a CSS animation so it doesn't do this effects.

Hey @anxolin thanks! we'll tackle this on #21.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
app:Explorer Explorer App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Batch Viewer] Improve Loading indicator
7 participants