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

WIP perf: add PureSkeletonContent component to improve performance by reducing re-rendering of component #29

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

mernxl
Copy link
Contributor

@mernxl mernxl commented Nov 19, 2020

This is a proof of concept as well as a final code to support this issue #28 i opened earlier.

As a guide, this section has been added to the README file.

PureSkeletonContent

If you are really concerned about performance, or you don't want your components mounting being runned while
isLoading is false, then you may need to consider using PureSkeletonContent.

Point to note

All props passed to PureSkeletonContent should be a constant prop, memoize it if it will change sometime.
Otherwise, you should consider using the good old SkeletonContent.

This point does not apply to componentProps as it is shallow checked to know if we should re-render the Skeleton or not.

import { FunctionComponent } from 'react';
import { Text, TextStyle, StyleSheet } from 'react-native';
import { PureSkeletonContent } from 'react-native-skeleton-content-nonexpo'; 

const Greetings: FunctionComponent<{ name: string }> = ({ name }) => 
  (<Text>Hello {name}</Text>);

const GreetingsSC = [{ height: 40, width: 200, paddingVertical: 2 }];

const SomeComponent: FunctionComponent<{name: string}> = ({ name }) => {
  const [loading, setLoading] = useState(true);

  return (
    <PureSkeletonContent 
      isLoading={isLoading}
      layout={GreetingsSC}
      component={Greetings} 
      componentProps={{ name }} // will be shallow checked, you don't need to memoize this
      containerStyle={styles.container} // notice we using styles from styleSheet
    />
  )
}

const styles = StyleSheet({
  container: {
    flex: 1, width: 300
  }
})

@mernxl mernxl marked this pull request as draft November 20, 2020 04:54
@mernxl mernxl marked this pull request as ready for review November 20, 2020 07:09
…functions for bones generation

BREAKING CHANGE: the main container no longer justifyContents and alignItems center.
The test around bones styles fails as there is a switch in Animated.View used.
@reichhartd
Copy link

@alexZajac @mernxl Is there an update here? Would be very interested in this feature.
Thanks for your effort!

@alexZajac
Copy link
Owner

alexZajac commented Sep 5, 2022

Thanks for the PR! We would need the checks to pass for it to be merged!

# Conflicts:
#	package-lock.json
#	package.json
#	src/Constants.ts
#	src/SkeletonContent.tsx
#	src/__tests__/SkeletonContent.test.tsx
#	src/__tests__/__snapshots__/SkeletonContent.test.tsx.snap
…g tests

BREAKING CHANGE: the main container no longer justifyContents and alignItems center.
The test around bones styles fails as there is a switch in Animated.View used.
@mernxl
Copy link
Contributor Author

mernxl commented Sep 16, 2022

Hi, this MR will need some more review and probably some talks. If possible @alexZajac could you test on a local project? It's been a while I worked on such a project.

I am available to help drive this towards a positive direction.

My initial change was we use Animated from react-native and ditch reanimated. But then it's been a while, it's possible there's been some updates that fixes a lot. Let's just take the time to review and see how to get to a good point.

@mernxl mernxl changed the title perf: add PureSkeletonContent component to improve performance by reducing re-rendering of component WIP perf: add PureSkeletonContent component to improve performance by reducing re-rendering of component Sep 16, 2022
@alexZajac
Copy link
Owner

Hi!

Yeah, this one slipped over my hands with the amount of work I had on the side, not sure if having a different component is the way to go here. My rationale is that if this one is "more performant" than the other SkeletonContent, then why don't we replace the implementation of this one instead? Maybe there is a functional difference I didn't catch there.

Thoughts?

@mernxl
Copy link
Contributor Author

mernxl commented Sep 19, 2022

Actually replacing without deprecating the current will make the community unstable. It will take time to migrate, most people will move away from the library. I would say I actually use them both within my project, for huge components, I will go with Pure, when I have small things to display within, I go with the current.

The primary function of the PureSkeletonContent is to allow you to pass in the "child component", it will only be initialized when isLoading is false and destroyed when true. I think a performance gain for navigating to new screens actually, where it does not create a component when it's not needed yet.

The current SkeletonContent, takes in an already initialized "child component", and so sometimes you need to check isLoading here and there especially when the data to be displayed drives the isLoading variable.

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