-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
refactor(plugin-ideal-image): migrate package to TS #5940
Conversation
✔️ [V2] 🔨 Explore the source changes: 5198a79 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/6193f65b59102500084539de 😎 Browse the preview: https://deploy-preview-5940--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5940--docusaurus-2.netlify.app/ |
The reason why we didn't migrate the theme components was because of swizzling (#5540 (comment)) I'll check if the output is nicer after the weekends. Overall I do think the current approach is more scalable than using Babel (just using tsc). Update. I took a look and currently the JS file is not usable for swizzling: Do you have any opinions about #5612? |
this output is way nicer than what is generated by #5612, all what we have to do is reformat code with eg prettier |
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
import React from 'react';
import ReactIdealImage from '@endiliey/react-ideal-image';
function IdealImage(props) {
const {alt, className, img} = props;
// In dev env just use regular img with original file
if (typeof img === 'string' || 'default' in img) {
return (
<img
src={typeof img === 'string' ? img : img.default}
className={className}
alt={alt}
{...props}
/>
);
}
return (
<ReactIdealImage
{...props}
alt={alt}
className={className}
height={img.src.height || 100}
width={img.src.width || 100}
placeholder={{lqip: img.preSrc}}
src={img.src.src}
srcSet={img.src.images.map((image) => ({
...image,
src: image.path,
}))}
/>
);
}
export default IdealImage; what do you think about output like this than? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, would just reduce public API surface
format?: 'webp' | 'jpeg' | 'png' | 'gif'; | ||
}; | ||
|
||
export type SrcImage = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering we don't even document those props anywhere, I'd suggest constraining the API to only what we document and not what is actually possible to pass.
Keeping a small API surface is likely to make it easier later to refactor/improve this image component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure what do you think we should expose in here, i actually limited it already to only what is supported by plugin
theoretically user can provide anything what is present in
https://github.com/endiliey/react-ideal-image/blob/de4e8f0388ac3645d3f32355c79c3b6a7cc61ff3/index.d.ts#L22-L88
unless you want me to roll-it back to img: any;
i don't see what i should do here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm, I think you are right, let's move on
the thing is, this plugin's prop is not really supposed to be crafted manually but should rather be obtained by an import/require + image loader so 🤷♂️ not sure how to enforce that
Yes that looks good to me. I think the Babel output could look better and preserve the line break though, but this is good enough. |
Motivation
#5817
Have you read the Contributing Guidelines on pull requests?
yes