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

Already on GitHub? Sign in to your account

use weekday fn from days.js to respect locale weekday reordering #46

Merged
merged 9 commits into from
Mar 2, 2020

Conversation

flo-l
Copy link
Contributor

@flo-l flo-l commented Mar 2, 2020

eg. MO-SO instead of SO-SA

@flo-l flo-l mentioned this pull request Mar 2, 2020
@flo-l
Copy link
Contributor Author

flo-l commented Mar 2, 2020

The tests are green on my box.


dayjs.extend(localeData);
dayjs.extend(localizedFormat);
dayjs.extend(weekday)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dayjs.extend(weekday)
dayjs.extend(weekday);

it's lint error, missing semicolon

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh thank you very much. I'll fix it asap!

@flo-l
Copy link
Contributor Author

flo-l commented Mar 2, 2020

I had to bump the dayjs dependency to get the weekday functionality.

The current version in the package is 1.8.12. Weekday came one release later, in 1.8.13. I updated to the latest version: 1.8.21. However, I can change that if you prefer.

@flo-l
Copy link
Contributor Author

flo-l commented Mar 2, 2020

hmm I can't run yarn install locally. I get an error during gyp compilation. @y0c could you please update the yarn lockfile?

@y0c
Copy link
Owner

y0c commented Mar 2, 2020

@flo-l I update yarn lockfile to dev branch.

@flo-l
Copy link
Contributor Author

flo-l commented Mar 2, 2020

thank you!

I'm afraid I can't build the current master on my machine. I get compile errors during yarn install. I'm on Fedora Linux with Node 12.15.

I can get the build to pass if I bump node-sass from 4.11.0 to 4.12.0.

@flo-l
Copy link
Contributor Author

flo-l commented Mar 2, 2020

thank you!

I'm afraid I can't build the current master on my machine. I get compile errors during yarn install. I'm on Fedora Linux with Node 12.15.

I can get the build to pass if I bump node-sass from 4.11.0 to 4.12.0.

Well I'm an idiot and didn't pull from your repo, but from my fork. It works now.

I changed the type of Locale from any to string | ILocale. If that is not what you intended I'll revert this change!

@y0c
Copy link
Owner

y0c commented Mar 2, 2020

Well done. Thank you.

@y0c y0c merged commit 9457c85 into y0c:dev Mar 2, 2020
@flo-l
Copy link
Contributor Author

flo-l commented Mar 2, 2020

Well done. Thank you.

Thank you too for your lib and the merge :)

@flo-l
Copy link
Contributor Author

flo-l commented Mar 2, 2020

One last request: Could you please do a release so I can use upstream in my project?

@y0c
Copy link
Owner

y0c commented Mar 2, 2020

@flo-l update version 1.0.3

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.

None yet

2 participants