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

Add type definitions #203

Open
StraightOuttaCrompton opened this issue Apr 14, 2020 · 7 comments
Open

Add type definitions #203

StraightOuttaCrompton opened this issue Apr 14, 2020 · 7 comments

Comments

@StraightOuttaCrompton
Copy link

Do you have any plan to support typescript by adding type definitions to the project?

@tkloht
Copy link
Owner

tkloht commented Apr 14, 2020

i'm considering doing that, but i don't have a timeline yet when this will happen.

i'll close this issue because it's not actionable for me right now, but please leave a comment if have anything to add, for example how this affects you right now. I'm not using typescript regularly right now, so i'd be interested to learn what is important for you in order to avoid mistakes once i add typescript support.

@tkloht tkloht closed this as completed Apr 14, 2020
@StraightOuttaCrompton
Copy link
Author

Ok, that's fair.

At the moment I am defining the types in a custom .d.ts which simple exports a ReactNode with any props. This just stops the TypeScript compiler complaining.

The most impactful thing for me I guess is that a library with type definitions is so much easier to consume, as I can immediately tell what props I can add to the exported component.

If you need any help later on give me a shout. I don't really have time right now to do this myself, but I don't mind contributing in the future.

@tkloht
Copy link
Owner

tkloht commented Apr 20, 2020

i actually did some work on this last weekend, and there is hope that you will get type definitions. i have a few things to work out, but if you want to check it out installing it like this should get you the typescript version: yarn add https://github.com/tkloht/react-video-cover.git#typescript. it would be very useful if you could let me know if it works. the branch is #204 . let's reopen this too

@tkloht tkloht reopened this Apr 20, 2020
@StraightOuttaCrompton
Copy link
Author

Nice! I just had a quick look and looks good. I will look into further tomorrow and get back to you!

@StraightOuttaCrompton
Copy link
Author

Hello. I have had a deeper look into your changes.

You shouldn't have to completely reimplement the library in TypeScript if you don't want to. You should be able to just define a index.d.ts in the src directory - like in this project.

If you were to do this then I would expect it to look something like the following

import { Component, CSSProperties } from 'react';
export declare type Props = {
    /**
     * This component will use object-fit: cover if available,
     * that is in all modern browsers except IE.
     * This prop forces use of the fallback. This is helpful during troubleshooting,
     * but apart from that you should not use it.
     * default: false
     */
    forceFallback?: boolean;
    /**
     * Additional styles which will be merged with those defined by this component.
     * Please note that some styles are not possible to override, in particular:
     *   - object-fit: cover (when the fallback is not used)
     *   - position: relative and overflow: hidden (when the fallback is used)
     */
    style?: CSSProperties;
    /**
     * Use this to set a custom className.
     */
    className?: string;
    /**
     * videoOptions will be passed as props to the <video/>.
     */
    videoOptions?: object;
    /**
     * If set, an event listener on window-resize is added when the Fallback is used.
     * It will re-evaluate the aspect-ratio and update the styles if necessary.
     * This has no effect if the fallback is not used.
     * The classic example where it makes sense to use this is when using a background video.
     * If you need to react to different events to re-measure the aspect-ratio
     * please see the onFallbackDidMount prop.
     * default: false
     */
    remeasureOnWindowResize?: boolean;
    /**
     * Will be executed when the Fallback is mounted.
     * The only parameter is a function, which can be used to force a re-measuring,
     * for example after the size of the surrounding container has changed.
     * Please note that this will only be invoked if the fallback is used, that is in IE.
     * See ResizableCoverExample for an example implementation.
     */
    onFallbackDidMount?: (updateContainerRatio: () => void) => void;
    /**
     * Will be executed before the Fallback unmounts.
     * You probably want to use this to clear any event-listeners added in onFallbackDidMount.
     */
    onFallbackWillUnmount?: () => void;
};

declare class VideoCover extends Component<Props> {}

export default VideoCover;

I have made each prop here optional as your library doesn't seem to require props. I would also expect the videoOptions property to implement an interface. Something like:

export interface IVideoOptions {
    src?: string;
    title?: string;
    ...
}

export declare type Props = {
    ...
    videoOptions?:  IVideoOptions;
    ...
}

However, if you do want to implement the library entirely in typescript then I have created a similar project to yours here for a reference.

@tkloht
Copy link
Owner

tkloht commented Apr 21, 2020

I was aware of that, although I overestimated how complicated it would be to add definitions when replying to your initial issue. I wanted to get a bit more comfortable with typescript for a while, so porting this library is mostly a learning project. Thanks for your definitions, will use this for reference

@StraightOuttaCrompton
Copy link
Author

Porting the library to TypeScript will definitely get you familiar with it. If you need anything from me or want to bounce some ideas of someone, give me a shout!

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

No branches or pull requests

2 participants