Skip to content

Fix/add locale option #43

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 25, 2022
Merged

Conversation

tomchentw
Copy link
Contributor

Closes #39 .

Notes

Good day, @uselessdev. Not until I tried to address #39 did I realize that the interfaces already have the weekStartsOn option. This PR adds the locale to the date-fn options along with some existing weekStartsOn option. I'm not sure if you like this approach though so please feel free to ditch this PR.

Although date-fns also supports specifying locale and weekStartsOn in the options in the same time, I personally prefer keeping only the locale option in the interface for simplicity. But this involves a breaking change so let's discuss it first.

Pros and Cons of Keeping Only the locale

Pros

  • Simplicity
  • Less cognitive overload
  • Align with date-fns

Cons

  • Breaking changes
  • Unable to customize if we don't want to pull in a locale object

@netlify
Copy link

netlify bot commented May 25, 2022

Deploy Preview for uselessdev-datepicker ready!

Name Link
🔨 Latest commit 660e9f7
🔍 Latest deploy log https://app.netlify.com/sites/uselessdev-datepicker/deploys/628e355009d4e8000817cb02
😎 Deploy Preview https://deploy-preview-43--uselessdev-datepicker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@tomchentw tomchentw force-pushed the fix/add-locale-option branch from 713ef0c to 8904e68 Compare May 25, 2022 06:51
@hiwllc
Copy link
Owner

hiwllc commented May 25, 2022

@tomchentw thanks again.

About the locale and weekStartsOn props I think we can keep the both options for now.

@tomchentw tomchentw force-pushed the fix/add-locale-option branch from 8904e68 to 660e9f7 Compare May 25, 2022 13:55
@tomchentw
Copy link
Contributor Author

@uselessdev I've updated the PR. Please review.

If it's not too much trouble, would you mind cutting a release after merging this?

Thank you 🙏

@hiwllc hiwllc merged commit 474bf02 into hiwllc:main May 25, 2022
@hiwllc
Copy link
Owner

hiwllc commented May 25, 2022

If it's not too much trouble, would you mind cutting a release after merging this?

Yes, I'll make a new release 😊

@hiwllc
Copy link
Owner

hiwllc commented May 25, 2022

@tomchentw tomchentw deleted the fix/add-locale-option branch May 25, 2022 15:43
@tomchentw
Copy link
Contributor Author

Thank you! 2.4.0 seems to work perfectly as expected. 😄

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

Successfully merging this pull request may close these issues.

Use the Locale on date-fns functions
2 participants