feat: glightbox types - Githubissues

biati-digital / glightbox

Pure Javascript lightbox with mobile support. It can handle images, videos with autoplay, inline content and iframes

MIT License

2.03k stars 228 forks source link

feat: glightbox types #411

Closed tomasvn closed 5 months ago

tomasvn commented 1 year ago

Draft PR for ts types

tomasvn commented 1 year ago

@gingerchew I have update the PR, can you check it out? I am not sure about the typings of methods and setting private fields

gingerchew commented 1 year ago

The GLightboxInit needs to be updated since the constructor only takes a config object. As for setting private methods, I think thats fine. Is there something that makes you doubt doing it that way?

gingerchew commented 1 year ago

I am no Typescript expert, so I'm interested in your opinion too

tomasvn commented 1 year ago

I am not sure about this method getElementIndex(node: Node) {...} about the type of the parameter, previously I had nothing, not sure if this is the correct type. Also how would one type the exported instance?

gingerchew commented 1 year ago

Re: the todo in the last commit, I think this would help https://www.typescriptlang.org/docs/handbook/utility-types.html#returntypetype

type init = ReturnType<(options: GLightboxConfig) => GLightboxInstance>

this could also be a wild goose chase.

gingerchew commented 1 year ago

I am not sure about this method getElementIndex(node: Node) {...} about the type of the parameter, previously I had nothing, not sure if this is the correct type. Also how would one type the exported instance?

I think Node works, since it's comparing against this.elements[n].node we should probably just make it the same type as that.

tomasvn commented 1 year ago

I have improved upon your suggestions, yeah haven't found other solution rarther than to use unions I have to think about, how it can be improved

tomasvn commented 1 year ago

Improved typings @gingerchew

gingerchew commented 1 year ago

My god, they’ve done it! I’ll double review it once I get free time again, but seriously didn’t think it would be that clean of a solution.

tomasvn commented 1 year ago

Had to go to reddit, not going to lie :D Well I had to reach out to better TS devs

gingerchew commented 1 year ago

Even still, great find!

I was looking through and found a place where the function arguments don't match up. Here on line 340 of the d.ts file, the open and openAt methods use the same arguments.

open should have an element and index arguments. While I think this would work:

open(element:Element, index:number)

both have default parameters of null, will typing it like above mess with anything?

open(element = null, startAt = null)

tomasvn commented 1 year ago

Even still, great find!

I was looking through and found a place where the function arguments don't match up. Here on line 340 of the d.ts file, the open and openAt methods use the same arguments.

open should have an element and index arguments. While I think this would work:
open(element:Element, index:number)
both have default parameters of null, will typing it like above mess with anything?
open(element = null, startAt = null)

Nice find, must have missed this one, no it should be fine

jmoratat commented 12 months ago

Hi, when will be available this PR?? Thank u!

gingerchew commented 11 months ago

The existing changes are probably safe to copy and paste into your own .d.ts file. There is more stuff internally that needs some additional typing before it makes sense to merge it in.

biati-digital commented 5 months ago

Hi everyone. Apologies for the delay. I've been working on v4, which got a total overhaul. Created a plugins API, revamped the website, docs, and more.

I'll be merging a few PRs and dropping the final update in the next days.