search

SteffeyDev / react-native-popover-view

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

fix: import ViewPropTypes #133

Closed hytStart closed 2 years ago

hytStart commented 2 years ago

I think it's better to use import { ViewPropTypes } from 'react-native' here.

SteffeyDev commented 2 years ago

Have you tested this on web? Pretty sure we did it the other way because that workaround was needed for web support. Might be fixed by now, haven't looked at RN web support for a while.

hytStart commented 2 years ago

@SteffeyDev Hi, here is my opinion. I mean 

// new
import { ViewPropTypes } from 'react-native'

const stylePropType =
  isWeb
    ? PropTypes.object
    : ViewPropTypes.style

not 

// old
const stylePropType =
  isWeb
    ? PropTypes.object
    : require('react-native').ViewPropTypes.style

Still have the condition of isWeb

It can be seen in this picture.

image

Actually, I don't understand the reason for require('react-native'), instead of using import { ViewPropTypes } from 'react-native'

SteffeyDev commented 2 years ago

Thanks for the details. Please review #86 for a discussion of why we went with the current approach. A lot of things should work in theory, but in practice things are messier.

SteffeyDev commented 2 years ago

I'm fine with this change as long as you boot up a react native web project with your change and show that your change doesn't break web (and ideally share that environment as well so that I can confirm).