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

Update CalendarMonthGrid.jsx #795

Merged
merged 6 commits into from
Nov 11, 2017
Merged

Update CalendarMonthGrid.jsx #795

merged 6 commits into from
Nov 11, 2017

Conversation

krtcom
Copy link
Contributor

@krtcom krtcom commented Oct 25, 2017

Makes sure months in state of CalendarMonthGrid are in current locale so whenever moment.locale() changes month names are rendered correctly.

I think it could solve issue 480 and maybe issue 637

Makes sure months in state are in current locale so whenever locale changes month names are rendered correctly
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 86.778% when pulling 2b4f752 on krtcom:patch-1 into ef98109 on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 86.778% when pulling dee4d9b on krtcom:patch-1 into b832da5 on airbnb:master.

@@ -146,6 +146,9 @@ class CalendarMonthGrid extends React.Component {
const withoutTransitionMonths = orientation === VERTICAL_SCROLLABLE;
newMonths = getMonths(initialMonth, numberOfMonths, withoutTransitionMonths);
}

var momentLocale = moment.locale();
newMonths = newMonths.map(m => m.locale(momentLocale));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to cache the locale in an instance variable and only do this if it has changed?

Also this is breaking some or our eslint rules (trailing spacings, no var, etc)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 86.796% when pulling 787eeec on krtcom:patch-1 into 25a65e9 on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 86.676% when pulling 2874fa0 on krtcom:patch-1 into 25a65e9 on airbnb:master.

var momentLocale = moment.locale();
newMonths = newMonths.map(m => m.locale(momentLocale));

if (this.locale !== moment.locale()) {
Copy link
Member

Choose a reason for hiding this comment

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

Let’s cache the call to locale here, so we don’t have to do it twice

Copy link

@humphries40 humphries40 Nov 8, 2017

Choose a reason for hiding this comment

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

@krtcom You still here? I'd love to have this updated, and I'm not exactly sure if I can add to your pull request, but I believe all you need to do is this.

var momentLocale = moment.locale();
if (this.locale !== momentLocale) {
this.locale = momentLocale;
newMonths = newMonths.map(m => m.locale(this.locale));
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@humphries40 if you want to cherry-pick this onto another branch and open a PR with the fix, we would def roll that in!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi there, sorry I was busy with something else, and postponing this one but I can find some time to change it this morning.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM.

This could use a rebase down to a single commit, as well, especially to get rid of merge commits.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 86.685% when pulling 5712da9 on krtcom:patch-1 into 44fa104 on airbnb:master.

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.

LGTM!

@majapw majapw merged commit ed4d069 into react-dates:master Nov 11, 2017
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

5 participants