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

[Feature] add "renderMonth" support + add Persian locale story #449

Merged
merged 2 commits into from
May 2, 2017

Conversation

cyclops24
Copy link

@cyclops24 cyclops24 commented Apr 14, 2017

This PR add renderMonth to react-dates and help to add custom calendar like Jalali (Persian) calendar in future.
It's needed for #447

This is a simple screenshot from story page:
jalali_5

@cyclops24 cyclops24 changed the title add "renderMonth" support + add Persian local story add "renderMonth" support + add Persian locale story Apr 14, 2017
@cyclops24 cyclops24 changed the title add "renderMonth" support + add Persian locale story [Feature] add "renderMonth" support + add Persian locale story Apr 14, 2017
@coveralls
Copy link

coveralls commented Apr 14, 2017

Coverage Status

Coverage remained the same at 83.961% when pulling 05490c0 on intwarehq:feature/renderMonth into 3815516 on airbnb:master.

@@ -104,7 +107,7 @@ export default class CalendarMonth extends React.Component {
} = this.props;

const { weeks } = this.state;
const monthTitle = month.format(monthFormat);
const monthTitle = renderMonth ? renderMonth(month) : month.format(monthFormat);
Copy link
Member

Choose a reason for hiding this comment

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

If we're calling a user function here, we'd need to somehow assert that monthTitle is a string, to match month.format() - and throw if not.

Copy link
Author

Choose a reason for hiding this comment

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

So what is your suggestion?
I do this part as a code that now exist in CalendarDay component:
https://github.com/airbnb/react-dates/blob/master/src/components/CalendarDay.jsx#L131

Copy link
Member

Choose a reason for hiding this comment

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

hm, ok, fair point. this can stay as-is, and we can address them all holistically in the future.

Copy link
Author

@cyclops24 cyclops24 Apr 15, 2017

Choose a reason for hiding this comment

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

OK, @ljharb So any other change needs for this PR?

@@ -1,5 +1,6 @@
import React from 'react';
import moment from 'moment';
import momentJalaali from 'moment-jalaali';
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be added as a dev dep?

Copy link
Author

@cyclops24 cyclops24 Apr 15, 2017

Choose a reason for hiding this comment

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

Yes, Sorry I missed it. It's added to devDependencies now. Please check it again.

@ljharb ljharb added the semver-minor: new stuff Any feature or API addition. label Apr 14, 2017
@coveralls
Copy link

coveralls commented Apr 15, 2017

Coverage Status

Coverage remained the same at 83.961% when pulling 06baeea on intwarehq:feature/renderMonth into 3815516 on airbnb:master.

@cyclops24
Copy link
Author

@majapw do you have any idea about this?

**CC: ** @ljharb

@majapw majapw mentioned this pull request May 2, 2017
Copy link
Collaborator

@majapw majapw left a comment

Choose a reason for hiding this comment

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

I added this same support for the DRP as well. This looks good!

@coveralls
Copy link

coveralls commented May 2, 2017

Coverage Status

Coverage remained the same at 83.961% when pulling 19f0af3 on intwarehq:feature/renderMonth into 767bebc on airbnb:master.

@majapw majapw merged commit 641ebd6 into react-dates:master May 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor: new stuff Any feature or API addition.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants