-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Convert some capital letters to lowercase and capitalize months and days in Spanish(Final) #16627
Convert some capital letters to lowercase and capitalize months and days in Spanish(Final) #16627
Conversation
@luacmartins @thesahindia One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@Julesssss @thesahindia @aldo-expensify new PR is ready for review with signed commit |
@@ -18,8 +18,8 @@ class CalendarPicker extends React.PureComponent { | |||
constructor(props) { | |||
super(props); | |||
|
|||
this.monthNames = moment.localeData(props.preferredLocale).months(); | |||
this.daysOfWeek = moment.localeData(props.preferredLocale).weekdays(); | |||
this.monthNames = _.map(moment.localeData(props.preferredLocale).months(), month => month.replace(/^\w/, c => c.toUpperCase())); |
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.
NAB: Just wondering why we didn't do something simpler like:
this.monthNames = _.map(moment.localeData(props.preferredLocale).months(), month => month.replace(/^\w/, c => c.toUpperCase())); | |
this.monthNames = _.map(moment.localeData(props.preferredLocale).months(), month => month.toUpperCase()); |
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.
@aldo-expensify The first line of code converts the month names to title case (where the first letter of each word is capitalized), while the second line of code converts the month names to all uppercase. We just need to convert first letter of the month to uppercase
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 see, thanks for explaining!
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.
Welcome
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 was also confused by this. Could you please add a small comment explaining this for people in the future? Thanks
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.
Does this mean that we want sentence case for these? If so, I believe that we can use Str.recapitalize for that.
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.
@ayazhussain79, can you make this 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.
@thesahindia ok I'm going to change 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.
you can check new 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.
Left a small comment, but thanks for getting the commits signed!
@@ -18,8 +18,8 @@ class CalendarPicker extends React.PureComponent { | |||
constructor(props) { | |||
super(props); | |||
|
|||
this.monthNames = moment.localeData(props.preferredLocale).months(); | |||
this.daysOfWeek = moment.localeData(props.preferredLocale).weekdays(); | |||
this.monthNames = _.map(moment.localeData(props.preferredLocale).months(), month => month.replace(/^\w/, c => c.toUpperCase())); |
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.
Does this mean that we want sentence case for these? If so, I believe that we can use Str.recapitalize for that.
@thesahindia you can check the new changes |
@@ -18,8 +19,8 @@ class CalendarPicker extends React.PureComponent { | |||
constructor(props) { | |||
super(props); | |||
|
|||
this.monthNames = moment.localeData(props.preferredLocale).months(); | |||
this.daysOfWeek = moment.localeData(props.preferredLocale).weekdays(); | |||
this.monthNames = _.map(moment.localeData(props.preferredLocale).months(), month => Str.recapitalize(month)); |
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.monthNames = _.map(moment.localeData(props.preferredLocale).months(), month => Str.recapitalize(month)); | |
this.monthNames = _.map(moment.localeData(props.preferredLocale).months(), Str.recapitalize); |
NAB
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.
@aldo-expensify changed that line as suggested you can check it
@ayazhussain79, the fixed issues section should look like this - Fixed Issues$ #16403 And you will need to update the steps. You can check other PRs to understand how we write the steps.
Here's how I would write the test steps for the first two cases. You can follow the same for other cases.
Lastly I would not suggest using commit message like "This commit will do xyz". For example in your first commit, instead of writing "This commit will capitalize month and day names" you could just write "capitalize month and day names". |
@thesahindia updated fixed issues section you can check it and next time I will correct all my mistakes, Thank you |
Reviewer Checklist
Screenshots/Videos |
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.
All yours @luacmartins
@ayazhussain79, what issue are you facing when using the app on iOS Safari?? |
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.
LGTM!
@Julesssss @aldo-expensify all yours |
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 all 👍
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.2.93-0 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.93-4 🚀
|
Details
We want to convert some capital characters to lowercase and capitalization for months and days in Spanish.
Fixed Issues
$ #16403
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
Offline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
2023-03-25-05-02-23_IjBKQjEb.mp4
Mobile Web - Chrome
Screen.Recording.2023-03-27.at.10.49.36.PM.mov
Mobile Web - Safari
Desktop
Screen.Recording.2023-03-27.at.10.40.34.PM.mov
iOS
Screen.Recording.2023-03-27.at.10.46.35.PM.mov
Android
Screen.Recording.2023-03-27.at.11.02.56.PM.mov