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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export functions on the utils module #14

Closed
arthurdenner opened this issue Apr 9, 2018 · 7 comments
Closed

Export functions on the utils module #14

arthurdenner opened this issue Apr 9, 2018 · 7 comments

Comments

@arthurdenner
Copy link
Contributor

Hello 馃憢

I'm building a datepicker on top of this library and wanted to know if it's possible to export the functions on the utils module. In my use case, I have a Today button and to make it work properly, I'm making use of the normalizeDate and isEqual functions (but copying them).

Would be nice to import them directly from dayzed. What do you think about it?

Thanks in advance.

@mkartchner994
Copy link
Contributor

mkartchner994 commented Apr 9, 2018

Hello @arthurdenner! Thanks for the interest!

I am hesitant to export internals of the lib. If we did that, it would make it harder in the future to change them since we wouldn't know at that point who would be relying on them. Instead, what I might like is to update those internal functions with the equivalents from the date-fns lib like startOfDay - https://date-fns.org/v1.29.0/docs/startOfDay which is equivalent to normalizeDate - and isEqual - https://date-fns.org/v1.29.0/docs/isEqual. That was suggested in a different issue - #10

This way there wouldn't be duplicate code for this logic if you wanted to implement it in your own component that is extending <Dayzed>. Plus we get the benefit of date-fns being a well tested lib.

What are you thoughts on something like this?

@arthurdenner
Copy link
Contributor Author

Thanks for the answer. Your hesitancy makes total sense and switch to date-fns would be a good solution. Can I work on a PR for this?

@mkartchner994
Copy link
Contributor

mkartchner994 commented Apr 9, 2018

Yes, a PR for this work would be great! If you are able to give me a day, I would like to add some tests to the lib that would make this refactor easier. I will ping you here when those tests have been added 馃憤

@arthurdenner
Copy link
Contributor Author

Nice, I'll wait for your signal then. Thanks!

@mkartchner994
Copy link
Contributor

@arthurdenner I think this is ready for you now. I added some integration tests around the lib using Cypress.io - #16. Let me know if you have any questions. Thanks!

@arthurdenner
Copy link
Contributor Author

Thanks, @mkartchner994. I'm not so familiar with publishing packages on npm, so I'm wondering, date-fns should be what which of dependency, a peerDependency?

@mkartchner994
Copy link
Contributor

@arthurdenner date-fns should be added to the dependencies since the methods that will be used from it will be packaged in with the <Dayzed> component dist bundle.

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