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

feat(celo-news): skeleton placeholder while loading #3274

Merged
merged 6 commits into from
Dec 23, 2022

Conversation

jeanregisser
Copy link
Member

@jeanregisser jeanregisser commented Dec 20, 2022

Description

This replaces the loading spinner by a skeleton placeholder with a shimmering animation in the Celo news feed.

Why?

  • Skeleton screens are perceived as being shorter in duration when compared against a blank screen and a spinner.
  • Use of loader / spinner creates a period of uncertainty for the user since the load time is unknown.
  • It drives user’s attention to progress instead of waiting time.
  • For creating an illusion of short page load times in apps and on the web.

From https://blog.prototypr.io/skeleton-loader-an-overview-purpose-usage-and-design-173b5340d0e1

Next: use this in other screens where it makes sense (transaction list, dapp list, etc).

I had a look at several libraries to implement this and settled on https://github.com/chramos/react-native-skeleton-placeholder as it had the "best" implementation using a mask and linear gradient so the effect works across several isolated views, with an animation driven by the native driver.
In the future, I could see us fork the lib or include it in our components to further customize it to our needs.

Test plan

screencap-2022-12-21T135359.194Z-1080.mp4

Note: I added an artificial delay of 5 secs in the above video so we can see it better

Related issues

Backwards compatibility

Yes

@codecov
Copy link

codecov bot commented Dec 21, 2022

Codecov Report

Merging #3274 (db4566c) into main (cfb8303) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3274   +/-   ##
=======================================
  Coverage   80.57%   80.58%           
=======================================
  Files         664      665    +1     
  Lines       23971    23977    +6     
  Branches     4315     4315           
=======================================
+ Hits        19314    19321    +7     
+ Misses       4591     4590    -1     
  Partials       66       66           
Impacted Files Coverage Δ
src/components/SkeletonPlaceholder.tsx 100.00% <100.00%> (ø)
src/exchange/CeloNewsFeed.tsx 96.82% <100.00%> (+0.10%) ⬆️
src/exchange/CeloNewsFeedItem.tsx 100.00% <100.00%> (ø)
src/tokens/utils.ts 94.11% <0.00%> (-5.89%) ⬇️
src/web3/saga.ts 80.80% <0.00%> (+3.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfb8303...db4566c. Read the comment docs.

@jeanregisser jeanregisser marked this pull request as ready for review December 21, 2022 14:29
Comment on lines +65 to +74
<View style={styles.contentContainer}>
<View style={{ ...styles.author, width: 60 }} />
<View style={styles.row}>
<View style={{ flex: 1 }}>
<View style={{ ...styles.title, flex: undefined, marginBottom: 4 }} />
<View style={{ ...styles.title, flex: undefined, width: '60%' }} />
</View>
<View style={styles.image} />
</View>
</View>
Copy link
Member Author

@jeanregisser jeanregisser Dec 21, 2022

Choose a reason for hiding this comment

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

I broke our rule of not using inline styles here because I thought it made more sense to have them directly there.
But let me know if you think I should really not do this.

Base automatically changed from jeanregisser/celo-news-config to main December 21, 2022 14:50
@jeanregisser jeanregisser force-pushed the jeanregisser/skeleton-placeholder branch from 0c3b7dd to 8d4dc8e Compare December 21, 2022 14:57
Copy link
Collaborator

@kathaypacific kathaypacific left a comment

Choose a reason for hiding this comment

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

LGTM!! any plans to submit the patch to the skeleton project? :D

@jeanregisser
Copy link
Member Author

Yes the patch was submitted already: chramos/react-native-skeleton-placeholder#71
I left a comment about this in it.

@jeanregisser jeanregisser enabled auto-merge (squash) December 23, 2022 09:46
@jeanregisser jeanregisser merged commit e1efe39 into main Dec 23, 2022
@jeanregisser jeanregisser deleted the jeanregisser/skeleton-placeholder branch December 23, 2022 10:24
@valora-inc valora-inc deleted a comment from linear bot Mar 8, 2023
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