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

feat:(image): add test #349

Merged
merged 39 commits into from
Jul 3, 2021
Merged

feat:(image): add test #349

merged 39 commits into from
Jul 3, 2021

Conversation

doom-9-zz
Copy link
Contributor

No description provided.

doom-9 and others added 30 commits June 17, 2021 17:42
@07akioni
Copy link
Collaborator

07akioni commented Jul 2, 2021

其实我仔细思考了一波,要不然把 alt 换成 image-props? 然后把这个 imageProps 在 <img /> 上展开。

@doom-9-zz
Copy link
Contributor Author

好的老哥

@07akioni
Copy link
Collaborator

07akioni commented Jul 2, 2021

好的老哥

PropType 类型可以用这个

import { ImgHTMLAttributes } from 'vue'

在这个 PR 改就行

@doom-9-zz
Copy link
Contributor Author

👌

@codecov
Copy link

codecov bot commented Jul 2, 2021

Codecov Report

Merging #349 (559d220) into main (3b6f421) will increase coverage by 0.43%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #349      +/-   ##
==========================================
+ Coverage   33.86%   34.29%   +0.43%     
==========================================
  Files         507      507              
  Lines       12099    12106       +7     
  Branches     3304     3310       +6     
==========================================
+ Hits         4097     4152      +55     
+ Misses       7215     7158      -57     
- Partials      787      796       +9     
Impacted Files Coverage Δ
src/image/src/Image.tsx 73.07% <100.00%> (+73.07%) ⬆️
src/image/src/ImageGroup.tsx 8.00% <0.00%> (+8.00%) ⬆️
src/image/src/ImagePreview.tsx 24.36% <0.00%> (+24.36%) ⬆️
src/image/styles/dark.ts 33.33% <0.00%> (+33.33%) ⬆️
src/image/src/icons.tsx 100.00% <0.00%> (+100.00%) ⬆️
src/image/styles/light.ts 100.00% <0.00%> (+100.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b6f421...559d220. Read the comment docs.

@@ -20,14 +29,24 @@ export default defineComponent({
props: imageProps,
setup (props) {
const imageRef = ref<HTMLImageElement | null>(null)
const imgPropsRef = props.imgProps || {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

这么些不行,如果一开始没有后来有了就出错了

imgPropsRef = toRef(props, 'imgProps')

props.height ? props.height : imgPropsRef.height
),
mergedSrc: computed(() => (props.src ? props.src : imgPropsRef.src)),
mergedAlt: computed(() => (props.alt ? props.alt : imgPropsRef.alt)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

38-45重复逻辑能否抽出一个函数来处理

Copy link
Collaborator

Choose a reason for hiding this comment

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

这一堆能不能用

pickImgKeys = [src,alt,height,width]
mergeProps(imgPropsRef.value,pickImgKeys.reduce(....))

来完成? @07akioni

Copy link
Collaborator

Choose a reason for hiding this comment

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

理论上是没问题的,不过这搞感觉也不会简单,我是感觉直接在 render 函数里合并就行了,不用造 computed

@doom-9-zz
Copy link
Contributor Author

image
当我把这个类型注释后,这个错误就消失了
image

@07akioni
Copy link
Collaborator

07akioni commented Jul 3, 2021

image
当我把这个类型注释后,这个错误就消失了
image

有点蛋疼,之前 inputProps 就没出这个问题

@07akioni
Copy link
Collaborator

07akioni commented Jul 3, 2021

先用普通的 object 吧

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants