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): preview-src prop #56

Merged
merged 1 commit into from
Dec 31, 2020

Conversation

wangcch
Copy link
Contributor

@wangcch wangcch commented Dec 30, 2020

Add preview-src to determine the original data.

<Image
  src="https://zos.alipayobjects.com/rmsportal/jkjgkEfvpUPVyRjUImniVslZfWPnJuuZ.png?x-oss-process=image/auto-orient,1/resize,p_10/quality,q_10"
  preview={{
    src: 'https://zos.alipayobjects.com/rmsportal/jkjgkEfvpUPVyRjUImniVslZfWPnJuuZ.png',
  }}
/>;

Fixes #55
Fixes ant-design/ant-design#28321

@vercel
Copy link

vercel bot commented Dec 30, 2020

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/react-component/image/cttgyj0ab
✅ Preview: https://image-git-fork-wangcch-preview-src.react-component.vercel.app

@codecov
Copy link

codecov bot commented Dec 30, 2020

Codecov Report

Merging #56 (d230eac) into master (9383a49) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #56   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          220       221    +1     
  Branches        57        58    +1     
=========================================
+ Hits           220       221    +1     
Impacted Files Coverage Δ
src/Image.tsx 100.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 9383a49...d230eac. Read the comment docs.

@shaodahong
Copy link
Member

It does not resolve #54, #54 is like Carousel

src/Image.tsx Outdated
@@ -52,6 +53,7 @@ const ImageInternal: CompoundedComponent<ImageProps> = ({
height,
style,
preview = true,
previewSrc,
Copy link
Member

Choose a reason for hiding this comment

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

This prop better put into 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.

👌ok

@wangcch
Copy link
Contributor Author

wangcch commented Dec 30, 2020

It does not resolve #54, #54 is like Carousel

837b4a2 to close #54

src/Preview.tsx Outdated

const { useState, useEffect } = React;

interface PreviewProps extends Omit<IDialogPropTypes, 'onClose'> {
onClose?: (e: React.SyntheticEvent<Element>) => void;
src?: string;
alt?: string;
/** thumbnail src value */
thumbnail?: string;
Copy link
Member

Choose a reason for hiding this comment

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

If Preview use thumbnail too weird

Kapture.2020-12-31.at.10.42.54.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅 Maybe my thumbnails are a bit extreme.

Currently, the replacement is done when the image is loaded to achieve fast switching.
And thumbnail will appear on the first load and before the load is completed. After that it won't even appear.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, but is not universal, load status maybe Image <Spin /> or some text, like placeholder prop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 expose the loading state? let me try

Copy link
Member

Choose a reason for hiding this comment

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

I mean this pr resolve Image src not same as preview src, ant other issues like Preview placeholder(if loading) we maybe create new or and Image placeholder is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

understand 👌
Not a good way. This PR will not process thumbnails / load state.
Hope to export preview-src.

eg: aliyun oss

<Image
  src="https://xxx.xx/xxx.png?x-oss-process=image/auto-orient,1/resize,p_10/quality,q_10"
  preview={{
    src: 'https://xxx.xx/xxx.png',
  }}
/>;

Thanks 🙆‍♂️

@shaodahong
Copy link
Member

It does not resolve #54, #54 is like Carousel

837b4a2 to close #54

No, #54 look like

Kapture.2020-12-31.at.14.07.46.mp4

@wangcch
Copy link
Contributor Author

wangcch commented Dec 31, 2020

It does not resolve #54, #54 is like Carousel

837b4a2 to close #54

No, #54 look like

Kapture.2020-12-31.at.14.07.46.mp4

🤦‍♂️ I understand

Copy link
Member

@shaodahong shaodahong left a comment

Choose a reason for hiding this comment

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

LGTM

@shaodahong shaodahong merged commit 32432eb into react-component:master Dec 31, 2020
@shaodahong
Copy link
Member

+ rc-image@4.5.0

Thanks

@shaodahong
Copy link
Member

and help create pr in antd upgrade version(base branch feature), and related doc/example

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.

Image 的预览功能期望可以独立成函数,类似 Modal.info()
2 participants