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 16 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
18 changes: 18 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,23 @@ 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 {dimensions, loading, error} = useImageDimensions(source)

if(loading || error || !dimensions) {
return null
}
const {width, height, aspectRatio} = dimensions
```

### `useKeyboard`

```js
Expand Down
22 changes: 12 additions & 10 deletions src/index.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import {useDimensions} from './useDimensions'
import {useAppState} from './useAppState'
import {useBackHandler} from './useBackHandler'
import {useCameraRoll} from './useCameraRoll'
import {useClipboard} from './useClipboard'
import {useAccessibilityInfo} from './useAccessibilityInfo'
import {useKeyboard} from './useKeyboard'
import {useInteractionManager} from './useInteractionManager'
import {useDeviceOrientation} from './useDeviceOrientation'
import {useLayout} from './useLayout'
import useDimensions from './useDimensions'
import useAppState from './useAppState'
import useBackHandler from './useBackHandler'
import useCameraRoll from './useCameraRoll'
import useClipboard from './useClipboard'
import useAccessibilityInfo from './useAccessibilityInfo'
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,
}
61 changes: 61 additions & 0 deletions src/useImageDimensions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
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 dimensions (width, height and aspect ratio).
*/
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 [[dimensions, error], setState] = useState<
[{width: number; height: number}?, Error?]
>([])
Greg-Bush marked this conversation as resolved.
Show resolved Hide resolved

useEffect(() => {
try {
if (typeof source === 'number') {
const {width, height} = Image.resolveAssetSource(source)
setState([{width, height}])
} else if (typeof source === 'object' && source.uri) {
setState([])
Image.getSize(
source.uri,
(width, height) => setState([{width, height}]),
e => setState([dimensions, e]),
Greg-Bush marked this conversation as resolved.
Show resolved Hide resolved
)
} else {
throw new Error('not implemented')
}
} catch (e) {
setState([dimensions, e])
}
// eslint-disable-next-line react-hooks/exhaustive-deps
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 {
dimensions:
dimensions &&
(Object.setPrototypeOf(dimensions, {
Greg-Bush marked this conversation as resolved.
Show resolved Hide resolved
get aspectRatio(): number {
const _this = this as any
return _this.width / _this.height
},
}) as {
/**
* width to height ratio
*/
readonly aspectRatio: number
width: number
height: number
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 making width and height readonly as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unlike 'aspectRatio', actually, these fields are not read-only.

Copy link
Member

Choose a reason for hiding this comment

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

I think it could be good to mark them as readonly so catch a potential error where the end user accidentally modifies them, since that will be reset each render loop. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what do you mean and why do you think that field change will cause anything?

}),
error,
get loading() {
return !this.dimensions && !this.error
},
}
}

export default useImageDimensions