Add Vite as dependency and an optional property to hide indicators

[danielgraziani91]

I needed this customization for my project, so I decided to open a PR. Hope you will accept it!

PS: I don't know why Vite wasn't in the dependencies but to me it seemed good to have it there. Maybe I'm wrong. Let me know!

[Hejtmus]

Thanks @danielgraziani91 for PR, really appreciate it!

But I have to reject it, don't get me wrong, I will explain.
First let's start with Vite, it is missing, because it is peer dependency of @sveltejs/kit, that means every package, which uses kit must have Vite installed, so adding it there is just redundancy (except usage where it would specify exact version of Vite).
Now to optional indicators, problem is, that I already implemented this before 6 hours and I'm waiting with release until I'll have resolved #49 and #47 (it should be today, 29th August). My solution is CSS based, I try to minimize variables needed for customization, as it is one of the best practices to keep code clean, that CSS class is inevitable, but hiding buttons using JS is possible to do with CSS as well.
I'm really happy that you opened code of this lib and implement this feature, but I can't approve it. I really admire will to help, if you want to contribute with code/ideas I'm sure I can find some work here, if you just need this feature, I can ensure you, that it will be out soon.

Have a nice day,
Filip.

[danielgraziani91]

Got it, it makes sense. I’m happy I proposed something good :) Can’t wait! Il giorno mar 29 ago 2023 alle 00:16 Filip Holčík ***@***.***> ha scritto:

…

Thanks @danielgraziani91 <https://github.com/danielgraziani91> for PR, really appreciate it! But I have to reject it, don't get me wrong, I will explain. First let's start with Vite, it is missing, because it is peer dependency of @sveltejs/kit <https://github.com/sveltejs/kit/blob/adb57b117da1f03ae363aad00b13f126c984f713/packages/kit/package.json#L46>, that means every package, which uses kit must have Vite installed, so adding it there is just redundancy (except usage where it would specify exact version of Vite). Now to optional indicators, problem is, that I already implemented this before 6 hours and I'm waiting with release until I'll have resolved #49 <#49> and #47 <#47> (it should be today, 29th August). My solution <#57 (comment)> is CSS based, I try to minimize variables needed for customization, as it is one of the best practices to keep code clean, that CSS class is inevitable, but hiding buttons using JS is possible to do with CSS as well. I'm really happy that you opened code of this lib and implement this feature, but I can't approve it. I really admire will to help, if you want to contribute with code/ideas I'm sure I can find some work here, if you just need this feature, I can ensure you, that it will be out soon. Have a nice day, Filip. — Reply to this email directly, view it on GitHub <#62 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOHMWHRL4EBML4GWSFFDEE3XXUKEVANCNFSM6AAAAAA4CADQEM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Hejtmus]

It's done, I will make release for you, to have it ASAP, then I'll release patch with proper documentation, you can look up usage in #57.

[danielgraziani91]

OMG! Thanks a lot, man 😄 Is there anything I can help you with?

[Hejtmus]

You are welcome @danielgraziani91!
There is plenty things I need help with 😅,

• Problem you highlighted - insufficient documentation, there is need to document exact behavior of this lib, I think you are not first, that encountered Can't run example code on new SvelteKit project #61, but you are first breve enough to ask about it. Fixing documentation means mentioning all not necessarily obvious caveats and improving demo page, I wanted to add code snippets and examples to it, so potential users can more easily see, what is possible with this lib, thus saving their time (we have to return the favor for devs creating libs we use :) ).
• Automated testing, this is big one, but it would help at least create issue, gather info how could test this lib (there is so much scenarios, mostly testing browser events), maybe look how other svelte libs have solved this problem, I think we should have at least component testing and end-to-end testing.
• There is 1 urgent bug in FullPageSlide scrolls down on mobile when tapped #58, person reporting it also created PR for solving it Closes issue #58 #59, but proposed solutions were not optimal and introduced new bugs. If you want, you can try to fix this bug, but I personally think, it could be time consuming to debug it.
• Also there is less urgent bug/feature Only one axis drag round up #48, I was analyzing it and found too few solutions I like, and I think this could result of you just losing your time and rejected PR (I already rejected my at least 10 approaches 😅😂)

Those 2 last issues can teach you a lot of knowledge, it taught me a lot when I was working on first stable version and I encountered number of bugs and inconsistencies between browsers, so I actually had to read web specifications and it's implementations in browser engines. The most welcomed issue to solve is that documentation, I have not even clue, how perfect docs could look like, but I think you as user of this lib, you are more aware of it.
I don't expect you to pick one, at leas you have overview of where is heading this lib. If you would like to at least partially solve one of this issues, you can do it partially, I could finish it. Also I have created CONTRIBUTIONS.md guide for contributing, so if you will contribute please comply to that guide.

Again, thanks for will to contribute, you are awesome, there is not much people like you (or at least you are first I know about during 3 years of maintaining svelte libs)!

Have a nice day,
Filip.

[danielgraziani91]

Got it! Since the time I have in these days I will work on the doc. I've just forked again master to be aligned and I will scout the lib for functionalities 😄 Hope to make a PR real soon!