tomzaku / react-native-shimmer-placeholder

Placeholder/ Skeleton of React Native
MIT License
1.06k stars 93 forks source link

Add testID for easier testing #57

Closed Zeikko closed 3 years ago

Zeikko commented 3 years ago

Add optional testID string prop.

This enables the shimmer to be queried with react-native-testing-library using the testID. For example if we have a component which includes this JSX:

export const ExampleComponent = () => {
  const { data, loading } = useSomeAsyncEffect()
  return (
    <>
      {loading && <ShimmerPlaceholder testID="loadingShimmer" />}
      {!loading && <Text>Some Text {data}</Text>}
    </>
  )
}

We can then wait for all the shimmers to disappear before testing the component contents:

 const { getByText, getAllByTestId } =  render(<ExampleComponent />)
 await waitForElementToBeRemoved(() => getAllByTestId('loadingShimmer'))
 getByText('Some Text')

This makes it easy to test that shims are rendered and removed in the correct place and time.

tomzaku commented 3 years ago

Hi Zeikko, Thank you for your contribution.

I'm thinking to use restProps for missing props (https://reactnative.dev/docs/view) and pass it into View.

What do you think?

Zeikko commented 3 years ago

Thanks for your reply @tomzaku. Passing more props to view is a great idea! I made a new commit just to make sure i understood what you mean and it works for me. Is this what you meant? I think it's a good solution to my issue but i think it has some issues:

  1. It might not be clear which props are passed to the view and which are not unless you read the source code.
  2. What happens if you need to later pass more props to other components deeper into the JSX? You'd need to add those to separate props, but then keep outer view props in the restProps and that would be inconsistent. I'm not sure if this is relevant in such a small codebase as this though.
  3. If you ever add more props to your component and they clash with the viewProps the component users may experience issues where props are being passed to a different place than they were in previous versions.

These issues could be prevented using a prop called viewProps, containerProps or something similar instead of the restProps. The restProps also has its benefits and i'm not sure which approach would be the best here. What shall we do? :)

tomzaku commented 3 years ago

Using containerProps is a good idea. Maybe we only need containerProps which is replaced restProps and shimmerContainerProps which is located at 92.

Can you help me add 2 props for this component?

tomzaku commented 3 years ago

The reason why need to add shimmerContainerProps is for testing if who would like to test the visibility of placeholder

Zeikko commented 3 years ago

Good point! The shimmerContainerProps will be relevant when using shimmer to render the children. I also added childrenContainerProps for consistency. How does it look? Is there still something to improve?

tomzaku commented 3 years ago

I'm good with this change. I will merge and deploy tmr

Thank you again

Zeikko commented 3 years ago

Great! Thank you for your work and fast responses! I'm will be waiting for the release.

Zeikko commented 3 years ago

Actually. One more question. Should i change the containerProps to be passed before the style prop? This way the containerProps can't overwrite the style prop.

tomzaku commented 3 years ago

Hi @Zeikko, I think it is okay. Usually, developers will not put style there. if they insist to put, they will see the change. And that will be okay.

Zeikko commented 3 years ago

Hi @tomzaku, As far as i have understood everything should be fine now. Are you able to merge and release soon? I'm looking forward to the new version. Thanks!

tomzaku commented 3 years ago

Merging now. Thank you

tomzaku commented 3 years ago

react-native-shimmer-placeholder 2.0.7 published 🎉

Panlichen1996 commented 4 months ago

Hi @Zeikko ,Could you help me? I am trying test childrenContainerProps,shimmerContainerProps,containerProps these three prop, Could you upload some demo show how to use this three props ?