-
Notifications
You must be signed in to change notification settings - Fork 82
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
Translate the component using locales and translations #129
base: master
Are you sure you want to change the base?
Conversation
Thanks for the new PR! I'm having a very busy week, but I'll review it when I get the chance. ⭐️ |
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.
Thanks for the change! I'm sorry it has taken a while to review. I have a few questions before I'm ready to merge it.
lib/js/index.js
Outdated
dateValidator: undefined | ||
dateValidator: undefined, | ||
okLabel: 'OK', | ||
cancelLabel: 'Cancel' |
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.
👍
Can you add these options to README.md?
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.
Sure!
lib/js/index.js
Outdated
this.value = moment(0); | ||
this.setDate(this.options.default); | ||
this.setTime(this.options.default); | ||
} else { | ||
this.setDate(this.value); |
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.
Thanks for this fix 👍
I applied this part of the patch straight to master to fix the issue. It was released in v2.4.0, where I credited you.
Can you remove it from this PR?
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.
Consider it done :)
lib/js/index.js
Outdated
@@ -202,7 +209,7 @@ class DateTimePicker extends Events { | |||
} | |||
|
|||
onChangeDate(dateString) { | |||
const newValue = moment(this.value); | |||
const newValue = moment(this.options.default); |
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.
This doesn't look right. Can you explain it?
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 did it because when you clicked on a different date it changes the language back to English. This is caused by using this.value
, which is the default moment object that has the English locale only available. this.options.default
is the received moment object and can containts, therefore, more languages.
However, as you said it doesnt look right. Looking deeper on the code I have found other cases where this unwelcome behaviour appear.
lib/js/index.js
Outdated
@@ -342,7 +349,7 @@ class DateTimePicker extends Events { | |||
setDate(date) { | |||
const m = moment(date); | |||
const month = m.format('MMM'); | |||
const day = m.format('Do'); | |||
const day = m.format('D'); |
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.
Whilst this change better matches the material "picker" examples, I think it makes the component feel visually unbalanced.
Can you add options for the number strings instead?
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.
Let me check if there is an option in moment that I can use depending on the language
initialValue: this.value | ||
initialValue: this.options.default, | ||
weekdayFormat: this.options.default.localeData().weekdaysMin(), | ||
weekStart: this.options.default.clone().weekday(0).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.
What do these two lines change?
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.
WeekDayFormat
changes the names of the days displayed on the calendar according to the language.
weekStart
sets the week start day according to the moment object received as parameter.
Fixes a bug where the picker would render a blank space instead of the date and time when the picker was closed then re-opened. Original credit: @Luisetelo in #129.
I want this ! 👍 |
Just wanted to say thanks for this @Luisetelo! |
Working on an improved version...! |
Hey there! Here is the new version of the feature. The changes I've made are:
Drawback Looking forward to hearing from you! |
Hey there!
I created this PR from my crazy idea of #127. In fact, it wasn't as crazy as I thought. Actually, you can create a picker passing a moment object as
default
paramater. All I did was using this moment object in the points where a translation is needed. As well, I included two new parameters in the constructor to translate the button labels.