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
[daterangepicker] Fixed minYear/maxYear properties for DateRangePicker #62557
Conversation
|
@WiseBird Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in this PR
Code ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. You can test the changes of this PR in the Playground. Status
Once every item on this list is checked, I'll ask you for permission to merge and publish the changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 62557,
"author": "WiseBird",
"headCommitOid": "8f20d3d59670d2815d3197ffab3210414fd156ad",
"mergeBaseOid": "41d1b9237e351312a70c1bfbb251159965ce0bac",
"lastPushDate": "2022-10-05T15:40:24.000Z",
"lastActivityDate": "2022-12-08T19:35:39.000Z",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Well-liked by everyone",
"pkgInfo": [
{
"name": "daterangepicker",
"kind": "edit",
"files": [
{
"path": "types/daterangepicker/daterangepicker-tests.ts",
"kind": "test"
},
{
"path": "types/daterangepicker/index.d.ts",
"kind": "definition"
}
],
"owners": [
"SirMartin",
"smasala",
"nertzy"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "sandersn",
"date": "2022-12-08T19:35:39.000Z",
"isMaintainer": true
},
{
"type": "approved",
"reviewer": "SirMartin",
"date": "2022-10-17T07:47:23.000Z",
"isMaintainer": false
},
{
"type": "approved",
"reviewer": "nertzy",
"date": "2022-10-06T14:24:28.000Z",
"isMaintainer": false
}
],
"mainBotCommentID": 1267135246,
"ciResult": "missing"
} |
|
🔔 @SirMartin @smasala @nertzy — please review this PR in the next few days. Be sure to explicitly select |
|
I see that the default value for those fields is a string. But, it also seems like they can be assigned as a number, from the following: https://github.com/dangrossman/daterangepicker/blob/8495717c4007a03fd5dee422f161811fd6140c0e/daterangepicker.js#L236-L241 Would it be better to use |
|
It is a year, so probably a number is the most exactly and it is supported. |
|
http://www.daterangepicker.com/#options There are numbers in the documentation. |
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.
Looks like the value for minYear and maxYear can be either a string or a number.
So we should use string | number here instead.
|
@WiseBird One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
|
@nertzy Done |
|
@nertzy I am sorry, who should merge the PR? |
|
I think it would usually get automatically merged once approved. I'm not sure who to ping to have this looked at... |
Please fill in this template.
npm test <package to test>.Select one of these and delete the others:
If adding a new definition:
.d.tsfiles generated via--declarationdts-gen --dt, not by basing it on an existing project.tslint.jsonshould contain{ "extends": "@definitelytyped/dtslint/dt.json" }, and no additional rules.tsconfig.jsonshould havenoImplicitAny,noImplicitThis,strictNullChecks, andstrictFunctionTypesset totrue.If changing an existing definition:
If removing a declaration:
notNeededPackages.json.