search

deseretdigital / dayzed

Primitives to build simple, flexible, WAI-ARIA compliant React date-picker components.
https://dayzed.netlify.app
MIT License
663 stars 27 forks source link

Export functions on the utils module #14

Closed arthurdenner closed 6 years ago

arthurdenner commented 6 years ago

Hello :wave:

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 commented 6 years ago

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 - https://github.com/deseretdigital/dayzed/issues/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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

mkartchner994 commented 6 years ago

@arthurdenner I think this is ready for you now. I added some integration tests around the lib using Cypress.io - https://github.com/deseretdigital/dayzed/pull/16. Let me know if you have any questions. Thanks!

arthurdenner commented 6 years ago

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 commented 6 years ago

@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.