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.

Sign up for GitHub

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

feat(image): support icon demo #2728

Merged
merged 4 commits into from
Nov 11, 2024
Merged

feat(image): support icon demo #2728

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8987e52
chore(image): icon demo
Alex-huxiyang Nov 11, 2024
bbe11b4
chore: update
Alex-huxiyang Nov 11, 2024
06e00a3
chore: update
Alex-huxiyang Nov 11, 2024
2752082
Merge branch 'V3.0' into hxy/image/review
xiaoyatong Nov 11, 2024
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: 6 additions & 7 deletions src/packages/image/demo.taro.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import Demo5 from './demos/taro/demo5'
import Demo6 from './demos/taro/demo6'
import Demo7 from './demos/taro/demo7'
import Demo8 from './demos/taro/demo8'
import { harmonyAndRn } from '@/utils/platform-taro'
import { harmony, harmonyAndRn } from '@/utils/platform-taro'

const ImageDemo = () => {
const [translated] = useTranslate({
Expand Down Expand Up @@ -49,19 +49,18 @@ const ImageDemo = () => {
<Cell>
<Demo2 />
</Cell>
{!harmonyAndRn() && (
<View className="h2">{translated.loading}</View>
<Cell>
<Demo3 />
</Cell>
{!harmony() && (
<>
<View className="h2">{translated.loading}</View>
<Cell>
<Demo3 />
</Cell>
<View className="h2">{translated.error}</View>
<Cell>
<Demo4 />
</Cell>
</>
)}

<View className="h2">{translated.imageText}</View>
<Cell>
<Demo5 />
Expand Down
3 changes: 1 addition & 2 deletions src/packages/image/demos/taro/demo1.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import React from 'react'
import { Image } from '@nutui/nutui-react-taro'
import pxTransform from '@/utils/px-transform'

const Demo1 = () => {
const src =
'https://storage.360buyimg.com/imgtools/e067cd5b69-07c864c0-dd02-11ed-8b2c-d7f58b17086a.png'
return (
<>
<Image src={src} height={pxTransform(200)} />
<Image src={src} height={200} />
</>
)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

缺少错误处理演示

考虑到 PR 的主要目标是解决 HarmonyOS 环境下的 onError 事件问题，建议在该演示组件中添加错误处理的示例。

const Demo1 = () => {
  const src =
    'https://storage.360buyimg.com/imgtools/e067cd5b69-07c864c0-dd02-11ed-8b2c-d7f58b17086a.png'
+ const onError = () => {
+   console.log('图片加载失败')
+ }
  return (
    <>
-      <Image src={src} height={200} />
+      <Image 
+        src={src} 
+        height={200}
+        onError={onError}
+        fallback="https://img11.360buyimg.com/imagetools/jfs/t1/137646/13/7132/1648/5f4c748bE43da8ddd/a3f06d51dcae7b60.png"
+      />
    </>
  )
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const Demo1 = () => {
const src =
'https://storage.360buyimg.com/imgtools/e067cd5b69-07c864c0-dd02-11ed-8b2c-d7f58b17086a.png'
return (
<>
<Image src={src} height={pxTransform(200)} />
<Image src={src} height={200} />
</>
)
const Demo1 = () => {
const src =
'https://storage.360buyimg.com/imgtools/e067cd5b69-07c864c0-dd02-11ed-8b2c-d7f58b17086a.png'
const onError = () => {
console.log('图片加载失败')
}
return (
<>
<Image
src={src}
height={200}
onError={onError}
fallback="https://img11.360buyimg.com/imagetools/jfs/t1/137646/13/7132/1648/5f4c748bE43da8ddd/a3f06d51dcae7b60.png"
/>
</>
)

}
Expand Down
18 changes: 9 additions & 9 deletions src/packages/image/demos/taro/demo2.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,27 +13,27 @@ const Demo2 = () => {
<Image
src={src}
mode="aspectFit"
width={pxTransform(80)}
height={pxTransform(80)}
radius={pxTransform(40)}
width={80}
height={80}
radius={40}
Comment on lines +16 to +18
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议移除显式的宽度和高度设置

根据代码评审反馈，不建议在演示代码中直接指定图片的宽度和高度。建议将尺寸控制逻辑移至组件内部实现。

建议应用以下更改：

-            width={80}
-            height={80}
-            radius={40}
+            radius={40}

Also applies to: 25-27, 34-36

/>
</View>
<View style={{ width: pxTransform(98) }}>
<Image
src={src}
mode="scaleToFill"
width={pxTransform(80)}
height={pxTransform(80)}
radius={pxTransform(40)}
width={80}
height={80}
radius={40}
/>
</View>
<View style={{ width: pxTransform(98) }}>
<Image
src={src}
mode="scaleToFill"
width={pxTransform(80)}
height={pxTransform(80)}
radius={pxTransform(10)}
width={80}
height={80}
radius={10}
/>
</View>
</View>
Expand Down
11 changes: 6 additions & 5 deletions src/packages/image/demos/taro/demo3.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react'
import { Image } from '@nutui/nutui-react-taro'
import { Loading } from '@nutui/icons-react-taro'
import { View } from '@tarojs/components'
import pxTransform from '@/utils/px-transform'

const Demo3 = () => {
const imageText: React.CSSProperties = {
Expand All @@ -13,14 +14,14 @@ const Demo3 = () => {
return (
<>
<View style={{ display: 'flex', flexWrap: 'wrap' }}>
<View style={{ width: 98 }}>
<Image width="80" height="80" />
<View style={{ width: pxTransform(98) }}>
<Image width={80} height={80} />
<View style={imageText}>默认</View>
</View>
<View style={{ width: 98 }}>
<View style={{ width: pxTransform(98) }}>
<Image
width="80"
height="80"
width={80}
height={80}
loading={<Loading className="nut-icon-loading" />}
/>
<View style={imageText}>自定义</View>
Expand Down
9 changes: 5 additions & 4 deletions src/packages/image/demos/taro/demo4.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react'
import { Image } from '@nutui/nutui-react-taro'
import { Failure } from '@nutui/icons-react-taro'
import { View } from '@tarojs/components'
import pxTransform from '@/utils/px-transform'

const Demo4 = () => {
const imageText: React.CSSProperties = {
Expand All @@ -13,12 +14,12 @@ const Demo4 = () => {
return (
<>
<View style={{ display: 'flex', flexWrap: 'wrap' }}>
<View style={{ width: 98 }}>
<Image src="https://x" width="80" height="80" />
<View style={{ width: pxTransform(98) }}>
<Image src="https://x" width={80} height={80} />
<View style={imageText}>默认</View>
</View>
<View style={{ width: 98 }}>
<Image src="https://x" width="80" height="80" error={<Failure />} />
<View style={{ width: pxTransform(98) }}>
<Image src="https://x" width={80} height={80} error={<Failure />} />
<View style={imageText}>自定义</View>
</View>
</View>
Expand Down
4 changes: 2 additions & 2 deletions src/packages/image/demos/taro/demo5.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ const Demo5 = () => {
<>
<Image
src="http://m.360buyimg.com/babel/s181x181_jfs/t1/210178/19/10205/31538/619bbcd9E5071aed5/8e1b7eb632aeed49.png"
width={pxTransform(30)}
height={pxTransform(30)}
width={30}
height={30}
Comment on lines +11 to +12
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议保持响应式设计的一致性

代码中存在以下问题：

  1. Image 组件使用固定像素值，而同文件中的 View 组件仍在使用 pxTransform
  2. 移除 pxTransform 可能影响组件在不同设备上的适配性

建议应用以下修改：

-        width={30}
-        height={30}
+        width={pxTransform(30)}
+        height={pxTransform(30)}

另外，这个改动似乎没有解决 PR 中提到的 HarmonyOS 环境下 onError 事件的问题。是否需要添加相关的错误处理逻辑？

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
width={30}
height={30}
width={pxTransform(30)}
height={pxTransform(30)}

/>
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议添加错误处理的演示用例

考虑到 PR 的主要目标是解决 HarmonyOS 环境下 onError 事件的问题，建议在 demo 中添加：

  1. 错误图片地址的测试用例
  2. onError 事件处理的演示
  3. 在 HarmonyOS 环境下的特殊处理逻辑

建议添加如下示例代码：

       <Image
-        src="http://m.360buyimg.com/babel/s181x181_jfs/t1/210178/19/10205/31538/619bbcd9E5071aed5/8e1b7eb632aeed49.png"
+        src="invalid-image-url"
         width={30}
         height={30}
+        onError={(e) => {
+          console.log('Image load failed:', e)
+        }}
       />

Committable suggestion skipped: line range outside the PR's diff.

<View
style={{
Expand Down
7 changes: 1 addition & 6 deletions src/packages/image/demos/taro/demo6.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,7 @@ const Demo6 = () => {
}}
key={mode}
>
<Image
src={src}
mode={mode as any}
width={pxTransform(80)}
height={pxTransform(80)}
/>
<Image src={src} mode={mode as any} width={80} height={80} />
</View>
)
})}
Expand Down
7 changes: 1 addition & 6 deletions src/packages/image/demos/taro/demo7.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,7 @@ const Demo7 = () => {
}}
key={mode}
>
<Image
src={src}
mode={mode as any}
width={pxTransform(80)}
height={pxTransform(80)}
/>
<Image src={src} mode={mode as any} width={80} height={80} />
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议统一使用 pxTransform 处理尺寸

当前实现中，外层 View 使用 pxTransform 处理尺寸，而 Image 组件直接使用固定数值，这样的处理方式不够统一。另外，根据评论建议，不需要为图片指定固定的宽高。

建议修改实现方式：

-<Image src={src} mode={mode as any} width={80} height={80} />
+<Image 
+  src={src} 
+  mode={mode as any} 
+  width={pxTransform(80)} 
+  height={pxTransform(80)} 
+/>

或者考虑完全移除固定尺寸，让图片自适应容器大小：

-<Image src={src} mode={mode as any} width={80} height={80} />
+<Image src={src} mode={mode as any} />
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<Image src={src} mode={mode as any} width={80} height={80} />
<Image
src={src}
mode={mode as any}
width={pxTransform(80)}
height={pxTransform(80)}
/>
```
Option 2 (removing fixed dimensions):
```suggestion
<Image src={src} mode={mode as any} />

</View>
)
})}
Expand Down
30 changes: 8 additions & 22 deletions src/packages/image/image.taro.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
import { Image as ImageIcon, ImageError } from '@nutui/icons-react-taro'
import classNames from 'classnames'
import { BaseEventOrig } from '@tarojs/components/types/common'
import { harmonyAndRn } from '@/utils/platform-taro'
import pxTransform from '@/utils/px-transform'

export interface ImageProps extends Omit<TImageProps, 'style'> {
style?: CSSProperties
Expand Down Expand Up @@ -50,7 +50,7 @@ export const Image: FunctionComponent<Partial<ImageProps>> = (props) => {
const [isError, setIsError] = useState(false)

const pxCheck = (value: string | number): string => {
return Number.isNaN(Number(value)) ? String(value) : `${value}px`
return Number.isNaN(Number(value)) ? String(value) : pxTransform(+value)
}

// 图片加载
Expand All @@ -69,21 +69,9 @@ export const Image: FunctionComponent<Partial<ImageProps>> = (props) => {

const containerStyle = {
// eslint-disable-next-line no-nested-ternary
height: height
? Taro.getEnv() === 'RN'
? height
: pxCheck(height)
: Taro.getEnv() === 'WEB'
? ''
: '100%',
height: height ? pxCheck(height) : Taro.getEnv() === 'WEB' ? '' : '100%',
// eslint-disable-next-line no-nested-ternary
width: width
? Taro.getEnv() === 'RN'
? width
: pxCheck(width)
: Taro.getEnv() === 'WEB'
? ''
: '100%',
width: width ? pxCheck(width) : Taro.getEnv() === 'WEB' ? '' : '100%',
overflow: radius !== undefined && radius !== null ? 'hidden' : '',
borderRadius:
// eslint-disable-next-line no-nested-ternary
Expand Down Expand Up @@ -139,12 +127,10 @@ export const Image: FunctionComponent<Partial<ImageProps>> = (props) => {
onLoad={(e) => handleLoad(e)}
onError={(e) => handleError(e)}
/>
{!harmonyAndRn() && (
<>
{renderLoading()}
{renderErrorImg()}
</>
)}
<>
{renderLoading()}
{renderErrorImg()}
</>
</View>
)
}
Expand Down
Loading