Skip to content

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