Conversation
alexeyMohnatkin
commented
Jun 6, 2017
alexeyMohnatkin
commented
- 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
| 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.
I meant the lodash/range 😉
There was a problem hiding this comment.
No. I can remove it tomorrow
|
is it ready to merge? |
nemoDreamer
left a comment
nemoDreamer
left a comment
There was a problem hiding this comment.
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.
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.
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.
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".
| 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.
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 |
3 participants
