-
Notifications
You must be signed in to change notification settings - Fork 172
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
locale support #71
base: master
Are you sure you want to change the base?
locale support #71
Conversation
alexeyMohnatkin
commented
Jun 6, 2017
- take day names and start day of the week from locale
- make variables names human readable
- use moment-range for building days range
- fix bug with extra week (on screenshot)
is something wrong with this pr? |
All things I was missing... Thanks!
|
Could you clean this pr a little bit? |
@alexeyMohnatkin looks like you made the changes in |
@nemoDreamer |
src/calendar.js
Outdated
@@ -1,32 +1,34 @@ | |||
import moment from 'moment'; | |||
import React, { Component } from 'react'; | |||
import cx from 'classnames'; | |||
import range from 'lodash/range'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant the lodash/range
😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I can remove it tomorrow
is it ready to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge is not up to me, @alexeyMohnatkin, but this now looks ready to go!
src/calendar.js
Outdated
const date = m.date(); | ||
|
||
const monthStartWeekDay = m.clone().startOf('month').format('e'); | ||
const monthEndWeekDay = m.clone().endOf('month').format('e'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These actually return strings, so I'm surprised that the ===
tests below work (although I can attest that they do...).
Local-aware weekday as a number can be done via .weekday()
(as opposed to non-locale .day()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moment().format('e')
returns number
You can change it to .weekday()
as it's more declarative if you wish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format('e')
-- as per documentation -- returns the weekday number as a String (all format methods return Strings).
When doing Math with that output, it's easy to introduce errors, since "1" + 1 === "11"
.
src/calendar.js
Outdated
const monthEndWeekDay = m.clone().endOf('month').format('e'); | ||
const dateStart = monthStartWeekDay === 0 ? m.clone().startOf('month') : m.clone().startOf('month').subtract(monthStartWeekDay, 'days'); | ||
const dateEnd = monthEndWeekDay === 6 ? m.clone().endOf('month') : m.clone().endOf('month').add(6-monthEndWeekDay, 'days'); | ||
const daysRange = moment.range(dateStart, dateEnd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: I used your locale changes in another project, but there I'd already split up all the logic into logic / presentational components, and only wanted the entry point to have a dependency on moment
. So I kept your locale stuff, and used the "old" lodash.range
way 😢
But for this self-contained single-component structure, it does make much more sense to let moment-range do the date-math 👍
fixed to |
sorry for typo, I've just noticed |