Open Myphz opened 1 month ago
@Myphz is attempting to deploy a commit to the darkroom Team on Vercel.
A member of the Team first needs to authorize it.
hey, thanks for taking the time on this! i am not sure we want to have all the typescript guards in the implementation. The typescript types are important for the api consumer not internals. a good choice may be defining types in a d.ts and leaving the core in js with jsdoc. I don't think @clementroche would like to use type guard like isHTMLElement. Will look review it closer when I have time for it. thanks tho.
hello @Myphz, thank you for your PR however i've just merged the snap branch into main. Can you adapt your PR accordingly ?
hey, thanks for taking the time on this! i am not sure we want to have all the typescript guards in the implementation. The typescript types are important for the api consumer not internals. a good choice may be defining types in a d.ts and leaving the core in js with jsdoc. I don't think @clementroche would like to use type guard like isHTMLElement. Will look review it closer when I have time for it. thanks tho.
Thanks for your feedback. My changes were mostly on the index.ts
file and the Lenis
class definition, which I believe is what is consumed by the APIs, not internally. Since that file was already using TypeScript, I figured it'd be helpful to add good type definitions to it. I didn't touch any other JavaScript file.
I will soon update the PR for the latest update. Let me know if there's a better way to handle this task. Thank you!
hey, thanks for taking the time on this! i am not sure we want to have all the typescript guards in the implementation. The typescript types are important for the api consumer not internals. a good choice may be defining types in a d.ts and leaving the core in js with jsdoc. I don't think @clementroche would like to use type guard like isHTMLElement. Will look review it closer when I have time for it. thanks tho.
Thanks for your feedback. My changes were mostly on the
index.ts
file and theLenis
class definition, which I believe is what is consumed by the APIs, not internally. Since that file was already using TypeScript, I figured it'd be helpful to add good type definitions to it. I didn't touch any other JavaScript file.I will soon update the PR for the latest update. Let me know if there's a better way to handle this task. Thank you!
yeah, makes sense. maybe a seperate d.ts file would make more sense. will have to talk this over. types seem mostly sound as far I can tell from the quick glance i took. good job.
I've updated the PR to the latest branch. This time, I made some changes to the Lenis class to simplify it a bit:
userData
parameter and this.userData
(apparently it was useless - it was never assigned)this.isTouching
removed (it was assigned but it was never used)this.hasScrolled
(it was used but never assigned, so it always resulted in undefined
- the assignment was commented)I may be wrong on these assumptions though - let me know your thoughts. Thanks
userData is necessary
userData is necessary
fixed
Thank you @Myphz, I will not merge your PR but use it as a base for improving types step by step to be sure that it doesn't break nothing and that I understand every choice you've made.
any
sThis PR should not affect the logic whatsoever. It should improve the DX for TypeScript users.