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 preview prop #939

Merged
merged 77 commits into from
Aug 25, 2021

Conversation

doom-9-zz
Copy link
Contributor

closes #922

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

vercel bot commented Aug 22, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tusimple/naive-ui/5XEHw9oo8eevuWtJjFWQHKoXuscA
✅ Preview: https://naive-ui-git-fork-doom-9-featimage-add-preview-prop-tusimple.vercel.app

console.log(document.querySelector('.n-image-preview')?.outerHTML)
// <img draggable="false" class="n-image-preview">
// 为什么没有src属性?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里应该怎么写?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

加了sleep也没用

Copy link
Collaborator

@XieZongChen XieZongChen Aug 22, 2021

Choose a reason for hiding this comment

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

可以在 props 里加入 attachTo: document.body 然后尝试是否能拿到想要的dom

Copy link
Collaborator

Choose a reason for hiding this comment

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

还有就是检查一下你的点击事件是否真的触发了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

可以拿到期望的dom,但是dom上缺少src属性,点击事件确实触发了,不然拿不到期望的dom

Copy link
Collaborator

Choose a reason for hiding this comment

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

在本地 DEMO work 吗?这个玩意吧,我也不知道 JSDOM 做了啥,你可以在谷歌搜搜 JSDOM img 相关的问题

Copy link
Collaborator

Choose a reason for hiding this comment

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

当然首先要确保代码是 work 的,可以在设定 URL 的地方打 log 看看发生了啥

@@ -61,14 +62,18 @@ export default defineComponent({
imgProps: imgPropsRef,
handleClick: () => {
if (imageGroupHandle) {
imageGroupHandle.setPreviewSrc(props.src)
imageGroupHandle.setPreviewSrc(
props.previewSrc ? props.previewSrc : props.src
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
props.previewSrc ? props.previewSrc : props.src
props.previewSrc || props.src

Comment on lines 74 to 76
previewInst.setPreviewSrc(
props.previewSrc ? props.previewSrc : props.src
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
previewInst.setPreviewSrc(
props.previewSrc ? props.previewSrc : props.src
)
previewInst.setPreviewSrc(
props.previewSrc || props.src
)

console.log(document.querySelector('.n-image-preview')?.outerHTML)
// <img draggable="false" class="n-image-preview">
// 为什么没有src属性?

Copy link
Collaborator

Choose a reason for hiding this comment

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

在本地 DEMO work 吗?这个玩意吧,我也不知道 JSDOM 做了啥,你可以在谷歌搜搜 JSDOM img 相关的问题

@kev1nzh37
Copy link
Collaborator

需要注意下 toolbar 中左右切换图的功能, go 函数也要兼容 preview-src。

// /image/src/ImageGroup.tsx
function go (step: 1 | -1): void {
      if (!vm?.proxy) return
      const container: HTMLElement = vm.proxy.$el.parentElement
      // use dom api since we can't get the correct order before all children are rendered
      const imgs = container.getElementsByClassName(
        groupId
      ) as HTMLCollectionOf<HTMLImageElement>
      if (!imgs.length) return
      const index = Array.from(imgs).findIndex((img) => img.src === currentSrc)
      if (~index) {
        setPreviewSrc(imgs[(index + step + imgs.length) % imgs.length].src)
      } else {
        setPreviewSrc(imgs[0].src)
      }
    }

@doom-9-zz
Copy link
Contributor Author

需要注意下 toolbar 中左右切换图的功能, go 函数也要兼容 preview-src。

// /image/src/ImageGroup.tsx
function go (step: 1 | -1): void {
      if (!vm?.proxy) return
      const container: HTMLElement = vm.proxy.$el.parentElement
      // use dom api since we can't get the correct order before all children are rendered
      const imgs = container.getElementsByClassName(
        groupId
      ) as HTMLCollectionOf<HTMLImageElement>
      if (!imgs.length) return
      const index = Array.from(imgs).findIndex((img) => img.src === currentSrc)
      if (~index) {
        setPreviewSrc(imgs[(index + step + imgs.length) % imgs.length].src)
      } else {
        setPreviewSrc(imgs[0].src)
      }
    }

需要放到img标签的一个属性上,有什么建议吗

@kev1nzh37
Copy link
Collaborator

需要放到img标签的一个属性上,有什么建议吗

应该没啥问题,当然也可以通过 Slots 来得到 image 组件拿到实例中的 preview-src。

@07akioni
Copy link
Collaborator

需要放到img标签的一个属性上,有什么建议吗

应该没啥问题,当然也可以通过 Slots 来得到 image 组件拿到实例中的 preview-src。

Slot 是不行的,放在孙子节点就找不到了

@kev1nzh37
Copy link
Collaborator

需要放到img标签的一个属性上,有什么建议吗

应该没啥问题,当然也可以通过 Slots 来得到 image 组件拿到实例中的 preview-src。

Slot 是不行的,放在孙子节点就找不到了

嗯我试了下并没啥好的办法。。只能当属性挂标签上了么

@doom-9-zz
Copy link
Contributor Author

doom-9-zz commented Aug 24, 2021

需要放到img标签的一个属性上,有什么建议吗

应该没啥问题,当然也可以通过 Slots 来得到 image 组件拿到实例中的 preview-src。

Slot 是不行的,放在孙子节点就找不到了

嗯我试了下并没啥好的办法。。只能当属性挂标签上了么

一张照片的时候没问题,就是用照片组的时候,也可以把preview-src属性直接传给n-image-group

@07akioni
Copy link
Collaborator

需要放到img标签的一个属性上,有什么建议吗

应该没啥问题,当然也可以通过 Slots 来得到 image 组件拿到实例中的 preview-src。

Slot 是不行的,放在孙子节点就找不到了

嗯我试了下并没啥好的办法。。只能当属性挂标签上了么

挂属性没啥毛病,是个合理的办法

@07akioni
Copy link
Collaborator

@doom-9 就挂在标签解决 go 的问题吧

@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #939 (59b758d) into main (dd92889) will increase coverage by 0.00%.
The diff coverage is 58.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #939   +/-   ##
=======================================
  Coverage   45.07%   45.07%           
=======================================
  Files         509      509           
  Lines       12464    12466    +2     
  Branches     3503     3506    +3     
=======================================
+ Hits         5618     5619    +1     
- Misses       5847     5848    +1     
  Partials      999      999           
Impacted Files Coverage Δ
src/image/src/ImageGroup.tsx 42.30% <0.00%> (-1.70%) ⬇️
src/image/src/Image.tsx 77.77% <87.50%> (+0.85%) ⬆️

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 dd92889...59b758d. Read the comment docs.

@doom-9-zz
Copy link
Contributor Author

@doom-9 就挂在标签解决 go 的问题吧

测试代码我先删除了,我自己测试是没问题的

@07akioni 07akioni merged commit 673a593 into tusen-ai:main Aug 25, 2021
rhengles pushed a commit to arijs/naive-ui that referenced this pull request Oct 20, 2021
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.

separate image preview src
5 participants