SteffeyDev / react-native-popover-view

A well-tested, adaptable, lightweight <Popover> component for react-native
MIT License
614 stars 92 forks source link

Please add testID prop to improve testability #126

Closed effinsky closed 2 years ago

effinsky commented 3 years ago

Please add testID prop to improve testability with jest and react testing library

SteffeyDev commented 3 years ago

Can you link to an example or docs about how you would use this prop? I'm not familiar with react testing library, and thus don't know what I would do with the value you would pass in this new prop.

effinsky commented 3 years ago

Hi, as far as I am aware now, a testID prop--standard for both native RN components and 3rd party libs--is pretty much the only way in this case to "grab" this component in a test so as to verify its behavior. TestID is not the optimal way to test components, but in this case I cannot see another way to refer to the popover directly so as to check its properties etc. For example, I can search for certain text that a child of Popover is supposed to render, but then I will have grabbed the component that has that text, not the Popover itself. Meanwhile, testID, if it were possible to give it to the Popover, would let me do that.

Screenshot 2021-11-09 at 20 16 09

https://reactnative.dev/docs/view#testid https://reactnativetesting.io/component/testing.html#interaction

Thanks in advance--

SteffeyDev commented 2 years ago

Ok, so it sounds like I just need to add the testID property to the TypeScript type so that you can pass it as a prop without it getting upset right? It doesn't look like I have to actually do anything with the string you pass in.

effinsky commented 2 years ago

Yes, it would seem so, thanks!

On 11/28/2021 9:26 PM, Peter Steffey wrote:

Ok, so it sounds like I just need to add the |testID| property to the TypeScript type so that you can pass it as a prop without it getting upset right? It doesn't look like I have to actually do anything with the string you pass in.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SteffeyDev/react-native-popover-view/issues/126#issuecomment-981146392, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANACK57YXF5372V5WACITG3UOKF7LANCNFSM5G66DDWA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

effinsky commented 2 years ago

Seems like it's not just a TS thing. Seems like you actually need to add the testID to the props accepted by the component, same as on the React Native's other built-in components and those provided by community libs like React Native Elements or Native Base. Thanks in advance!

SteffeyDev commented 2 years ago

So, technically React and JS allows you to pass any props to a component, because props is just a JS object, so there is no concept of accepting a prop. I can choose to read the value of a prop, but if I'm not going to do anything with it there's no need to read it. TypeScript is the only part of this that doesn't allow props to be passed in unless they are defined within the component. Hopefully that makes sense.

If you want me to implement it the same way as another open source project, please link me to the place in their code where they use this prop so that I can see what they are doing.

SteffeyDev commented 2 years ago

@effinsky I see examples like this where they add a testID prop to an underlying component, such as a view, so that the view can be grabbed. This is very different from adding a testID prop to the Popover component itself. Is this still a feature you are looking for?

ginnymin commented 2 years ago

I can't speak for the OP, but I came across this thread when searching for a way to do the same thing. Perhaps PopoverProps can extend off of ViewProps so any "common" props (such as testID or accessibilityRole) can be applied to the Popover component as well? I don't think applying a testID to a Popover child would help the case I'm trying to cover (that some values I'm calculating are being passed as expected to <Popover> specifically)

SteffeyDev commented 2 years ago

I don't want to accept arbitrary view props, but I'm fine with accepting a subset. So, for your use case, would it be sufficient to just include Pick<ViewProps, 'testID'> into the public Popover props?

ginnymin commented 2 years ago

That would work for me! Thanks so much

SteffeyDev commented 2 years ago

Ok, released as 5.1.4

SteffeyDev commented 2 years ago

Feel free to re-open if this doesn't work for you @ginnymin

myou11 commented 1 year ago

Hi @effinsky and @ginnymin. Sorry that this is unrelated to this issue, but have either of you successfully tested this Popover component with react-native-testing-library? I created issue #159 since I wasn't able to get it working, but was hoping you could share some advice if you did get it working

ginnymin commented 1 year ago

@myou11 I haven't tested this update yet, sorry I can't be of any help yet!