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

useImageDimensions #87

Merged
merged 22 commits into from
Apr 16, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ yarn add @react-native-community/hooks
- [useCameraRoll](https://github.com/react-native-community/hooks#usecameraroll)
- [useClipboard](https://github.com/react-native-community/hooks#useclipboard)
- [useDimensions](https://github.com/react-native-community/hooks#usedimensions)
- [useImageDimensions](https://github.com/react-native-community/hooks#useImageDimensions)
- [useKeyboard](https://github.com/react-native-community/hooks#usekeyboard)
- [useInteractionManager](https://github.com/react-native-community/hooks#useinteractionmanager)
- [useDeviceOrientation](https://github.com/react-native-community/hooks#usedeviceorientation)
Expand Down Expand Up @@ -104,6 +105,18 @@ const { width, height } = useDimensions().window
const screen = useDimensions().screen
```

### `useImageDimensions`

```js
import {useImageDimensions} from '@react-native-community/hooks'

const source = require('./assets/yourImage.png')
// or
const source = {uri: 'https://your.image.URI'}

const {width, height, loading, error} = useImageDimensions(source)
```

### `useKeyboard`

```js
Expand Down
2 changes: 2 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import useKeyboard from './useKeyboard'
import useInteractionManager from './useInteractionManager'
import useDeviceOrientation from './useDeviceOrientation'
import useLayout from './useLayout'
import useImageDimensions from './useImageDimensions'
LinusU marked this conversation as resolved.
Show resolved Hide resolved

export {
useDimensions,
Expand All @@ -20,4 +21,5 @@ export {
useInteractionManager,
useDeviceOrientation,
useLayout,
useImageDimensions,
}
36 changes: 36 additions & 0 deletions src/useImageDimensions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import {useEffect, useState} from 'react'
import {Image, ImageRequireSource} from 'react-native'

export interface URISource {
LinusU marked this conversation as resolved.
Show resolved Hide resolved
uri: string
}

/**
* @param source either a remote URL or a local file resource.
* @returns original image width and height.
*/
function useImageDimensions(source: ImageRequireSource | URISource) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about importing ImageSourcePropType from react-native and using that as the type for source?

Suggested change
function useImageDimensions(source: ImageRequireSource | URISource) {
function useImageDimensions(source: ImageSourcePropType) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of redundant properties in ImageSourcePropType too, that are not using in the hook.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any downside to using ImageSourcePropType, even though it has some more props defined though? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is about ImageSourcePropType:
https://reactnative.dev/docs/image#source
This prop can also contain several remote URLs, specified together with their width and height and potentially with scale/other URI arguments. The native side will then choose the best uri to display based on the measured size of the image container.
It means that ImageSourcePropType contains redundant types. No sense to use these types in this hook.

const [state, setState] = useState<{
width?: number
height?: number
loading?: boolean
Greg-Bush marked this conversation as resolved.
Show resolved Hide resolved
error?: any
Greg-Bush marked this conversation as resolved.
Show resolved Hide resolved
}>({})
Greg-Bush marked this conversation as resolved.
Show resolved Hide resolved
useEffect(() => {
if (typeof source === 'object' && typeof source.uri === 'string') {
Greg-Bush marked this conversation as resolved.
Show resolved Hide resolved
setState({loading: true})
Image.getSize(
source.uri,
(width, height) => setState({width, height}),
Greg-Bush marked this conversation as resolved.
Show resolved Hide resolved
error => setState({error}),
)
} else if (typeof source === 'number') {
setState(Image.resolveAssetSource(source))
Greg-Bush marked this conversation as resolved.
Show resolved Hide resolved
} else {
setState({error: 'not implemented'})
Greg-Bush marked this conversation as resolved.
Show resolved Hide resolved
}
}, [source])
Copy link

@tuantranailo tuantranailo Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question here: if the source is an object, wouldn't it be changed every render, triggering the warning: Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or **one of the dependencies changes on every render.**? This is because useEffect hook does shallow comparison, not deep comparison. @Greg-Bush and @LinusU

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on how you pass in the source, as long as the same object is being passed each render cycle it will not trigger the useEffect to re-run. The one case I can see would be problematic is if people pass the url as such:

useImageDimensions({ uri: 'https://...' })

We could help with this by doing something like:

-  }, [source])
+  }, [source?.uri ?? source])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LinusU Yes. That would definitely do the trick. 👍 👍 👍

return state
}

export default useImageDimensions