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

Allow 'today' modifier to override default #279

Merged
merged 2 commits into from
Mar 8, 2017
Merged

Conversation

maxdubrinsky
Copy link

The product I am working on allows users to specify a timezone other than that of their browsers, and in doing so might cause what the day picker component thinks today is to be different from what the user expects. This change simply allows me to override the today modifier with a custom function that could use other references, such as moment-timezone objects.

This was run with test, cover, and lint, but if there are other modifications necessary, please let me know.

Thanks, and I welcome your feedback!

src/DayPicker.js Outdated
let dayModifiers = [];
if (DateUtils.isSameDay(day, new Date())) {
dayModifiers.push(this.props.classNames.today);
const { today } = this.props.classNames;
Copy link
Owner

Choose a reason for hiding this comment

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

@maxdubrinsky could you change the name of this var to todayModifier? So it's easier to understand what it is... thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing.

src/DayPicker.js Outdated
const { today } = this.props.classNames;
const propModifiers = Helpers.getModifiersFromProps(this.props);
const dayModifiers = Helpers.getModifiersForDay(day, propModifiers);
if (DateUtils.isSameDay(day, new Date()) && !propModifiers.hasOwnProperty(today)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't use hasOwnProperty this way or the lint will complain:

- propModifiers.hasOwnProperty(today)
+ Object.prototype.hasOwnProperty.call(propModifiers, today)

thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Gotcha. Just about to ask how you guys wanted that handled syntacticly. I would normally use in, but there are a couple of reasons not to for performance reasons.

Copy link
Owner

Choose a reason for hiding this comment

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

even if uglier I'd just use Object.prototype.hasOwnProperty :)

@gpbl
Copy link
Owner

gpbl commented Mar 8, 2017

Good idea, thanks – do you feel to add a test as well? 👍

@gpbl
Copy link
Owner

gpbl commented Mar 8, 2017

This was run with test, cover, and lint, but if there are other modifications necessary, please let me know.

Weird it didn't catch the lint, did you run the folowing?

npm run lint

@maxdubrinsky
Copy link
Author

I did, but apparently I added that after I ran lint the first few times. I can throw a couple tests in, though.

@gpbl
Copy link
Owner

gpbl commented Mar 8, 2017

@maxdubrinsky are you working on this during the next... hours 😊? I'd like to release an update soon 🚀
Otherwise I can deal with it.

@maxdubrinsky
Copy link
Author

Sorry for the delay, I will be able to push an update in the next hour or so.

@gpbl
Copy link
Owner

gpbl commented Mar 8, 2017

No problem, thanks for your effort ❤️

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 97c1c2f on maxdubrinsky:master into c9ab28f on gpbl:master.

@gpbl gpbl merged commit d4ec272 into gpbl:master Mar 8, 2017
@gpbl
Copy link
Owner

gpbl commented Mar 8, 2017

Awesome thanks!

@maxdubrinsky
Copy link
Author

Thanks for being so prompt! Saved me a bunch of time and headache

@gpbl
Copy link
Owner

gpbl commented Mar 9, 2017

Thanks, published as v5.2.0 🎉

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.

3 participants