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

Fix add missing typescript definitions #201

Closed

Conversation

eyalktCloudinary
Copy link
Contributor

No description provided.

@eyalktCloudinary eyalktCloudinary requested review from strausr and a user December 4, 2019 12:35
Comment on lines +441 to +442
* @param {string} [publicId]
* @param {Object} [options]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used the same format as the ImageTag class (copy-paste), do you want me to add a more descriptive line like lines 724-725?

Comment on lines +444 to +447
export class SourceTag extends HtmlTag {
static "new"(publicId: string, options?: Transformation.Options): SourceTag;
static "new"(name: string, publicId: string, options?: Transformation.Options): SourceTag;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@eyalktCloudinary , how do I use this sourceTag? in pictureTag? in videoTag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A SourceTag instance is returned by sourceTag() (cloudinary.js), though I couldn't find any invocations of this function. The client that has raised this issue has found the function in our code and tries to use it, but couldn't - #189

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm guessing we're supposed to use it in pictureTag to create
<picture><source></source></picture>
but it seems that we're not

Copy link
Contributor

Choose a reason for hiding this comment

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

@eyalktCloudinary , let's check how this is used before we merge the PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is an internal class was exposed(alone and as a builder), but was never documented.

We use new SourceTag() in pictureTag, but I don't think we ever meant for the sourceTag itself to work in isolation, and for it to be public.

@ghost
Copy link

ghost commented Feb 17, 2020

Closing as this will need to be redone anyway after picture-tag is updated

@ghost ghost closed this Feb 17, 2020
@ghost ghost deleted the fix/add-missing-typescript-defenitions branch July 1, 2020 10:00
This pull request was closed.
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.

3 participants