natysoz / expo-images-picker

Multiple Asset Photos Videos selecting package for Expo SDK
MIT License
95 stars 35 forks source link

Use with header from StackNavigator #36

Closed shanedaugherty closed 2 years ago

shanedaugherty commented 3 years ago

We would like to use this with the default header from @react-navigation/native, is there a way to do that already that maybe we are missing?

From the looks of things to make that possible we would either need to:

const MyView = () => { const t = useTranslation() const navigator = useNavigator()

const assetsSelectorContext = useAssetsSelector({ Settings={widgetSettings} Errors={widgetErrors} Styles={widgetStyles} Resize={widgetResize}
Navigator={widgetNavigator} CustomNavigator={customNavigator} })

const onSuccess = (data: Asset[]) => { //... here we now have access to the assetSelectorContext which can contain the selected item state }

// making this all up for the sake of time, but you get the idea ;) // when the selected items state changes, update the built in header useEffect(() => { navigator.setOptions({ title: selected: ${assetSelectorContext?.selected?.length ?? 0}, headerRight: () => ( <Button disabled={assetSelectorContext?.selected?.length < 1} onPress={onSuccess}> Continue ) }) }, [assetSelectorContext?.selected])

return (<AssetsSelector {...assetsSelectorContext} />) }

natysoz commented 3 years ago

this useEffect might kill performances , did you try fork and do those changes ?

shanedaugherty commented 3 years ago

Hey @natysoz!

I did not try the example posted above because it requires a bit more work. It would require us to refactor the codebase to expose a hook that can be used to access all of the state / functions.

If we do go ahead with that approach, I would not worry about the performance of the useEffect in the example. It is a pretty lightweight effect that will only run when the selected items change. Keep in mind, this useEffect is in the application which is using the library -- not in the library itself.

I did go ahead and open a PR that adds an onChange prop. This was an easy change that allows a consumer of this library to access the state of which items are selected outside of the component. I have forked it and am using it in my project!

natysoz commented 3 years ago

we can create a custom hook , like useSelectedImages() where this hook return the selected items,but yhea it require to expose this function and bind it inside and along the code

shanedaugherty commented 3 years ago

Yep yep. I personally will not have the time to add this hook for a couple of months.

This pull request adds an onChange prop which also solves the same problem but in a different way: https://github.com/natysoz/expo-images-picker/pull/37

I suggest we do a minor release with the onChange prop and save the hook refactor for the next major version. What are your thoughts?

natysoz commented 3 years ago

i read the pr i dont understand what ur trying to return there on the onChange function ?

shanedaugherty commented 3 years ago

@natysoz the onChange function allows the component state to be hoisted outside of the component. For example

const MyView = ()=> {
  const [selectedImages, setSelectedImages] = useState<Asset>([])

  // I have access to the `selectedImages` state here now, outside of the component 

  return (
    <>
      {selectedImages?.length} selcted
      <ImagePicker onChange={setSelectedImages} />
    </>
  )
}
iammh93 commented 2 years ago

Seem like a good enhancement and provide much more flexibility. Looking forward to use it after PR merged

natysoz commented 2 years ago

ill work on such hook soon as im busy with new kid :) if someone wanna contribute in the meanwhile its not so hard to make

just need to test the performances

natysoz commented 2 years ago

added to the feature list