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
[@testing-library/jest-dom] Add types for toHaveDisplayValue #44385
[@testing-library/jest-dom] Add types for toHaveDisplayValue #44385
Conversation
@berickson1 Thank you for submitting 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. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 44385,
"author": "berickson1",
"owners": [
"gnapse",
"jgoz",
"smacpherson64"
],
"dangerLevel": "ScopedAndTested",
"headCommitAbbrOid": "30da26c",
"headCommitOid": "30da26cf53f3713c0d4a812b67c5eb55d407486c",
"mergeIsRequested": true,
"stalenessInDays": 6,
"lastCommitDate": "2020-05-12T00:33:09.000Z",
"reopenedDate": "2020-05-12T00:28:40.000Z",
"lastCommentDate": "2020-05-18T04:54:44.000Z",
"reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/44385/files",
"hasMergeConflict": false,
"authorIsOwner": false,
"isFirstContribution": false,
"popularityLevel": "Popular",
"anyPackageIsNew": false,
"packages": [
"testing-library__jest-dom"
],
"files": [
{
"filePath": "types/testing-library__jest-dom/index.d.ts",
"kind": "definition",
"package": "testing-library__jest-dom"
},
{
"filePath": "types/testing-library__jest-dom/test/testing-library__jest-dom-global-tests.ts",
"kind": "test",
"package": "testing-library__jest-dom"
}
],
"hasDismissedReview": false,
"travisResult": "pass",
"lastReviewDate": "2020-05-18T04:30:00.000Z",
"reviewersWithStaleReviews": [
{
"reviewedAbbrOid": "3a7d361",
"reviewer": "jgoz",
"date": "2020-05-08T19:46:30Z"
},
{
"reviewedAbbrOid": "7568be7",
"reviewer": "majesticuser",
"date": "2020-05-02T10:33:55Z"
},
{
"reviewedAbbrOid": "68aefa0",
"reviewer": "orta",
"date": "2020-04-30T17:07:25Z"
}
],
"approvalFlags": 2,
"isChangesRequested": false
} |
* @see | ||
* [testing-library/jest-dom#tohavedisplayvalue](https:github.com/testing-library/jest-dom#tohavedisplayvalue) | ||
*/ | ||
toHaveDisplayValue(value?: string | string[]): R; |
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.
nice work on those docs!
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.
Copy/pasted from their github docs, like the above examples :)
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.
It's the best way to be, my first DT PR were the exact same for Jest: #14345
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.
The case toHaveDisplayValue()
without an argument does not work. I get the following error:
TypeError: Cannot read property 'toString' of undefined
Also see the jest-dom documentation: https://github.com/testing-library/jest-dom#tohavedisplayvalue
I don't use this library, only creating for from github issue request where I was (incorrectly) mentioned. I asked @majesticuser to help validate |
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we? Comparison details 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
Hi, I'am sorry that I thought you was the author of this type definitions. And thank you very much for fixing the issue. I tried out your type definition file. Also see the jest-dom documentation: https://github.com/testing-library/jest-dom#tohavedisplayvalue |
🔔 @gnapse @jgoz @smacpherson64 - please review this PR in the next few days. Be sure to explicitly select |
* @see | ||
* [testing-library/jest-dom#tohavedisplayvalue](https:github.com/testing-library/jest-dom#tohavedisplayvalue) | ||
*/ | ||
toHaveDisplayValue(value?: string | string[]): R; |
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.
The case toHaveDisplayValue()
without an argument does not work. I get the following error:
TypeError: Cannot read property 'toString' of undefined
Also see the jest-dom documentation: https://github.com/testing-library/jest-dom#tohavedisplayvalue
@berickson1 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 or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you! |
1 similar comment
@berickson1 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 or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you! |
Give it another shot - I updated the definition |
@berickson1 can you please explain how typings for |
The github conflict markers for the same file segment looked like it was the same change + I misread the title - re-opening and fixing conflicts |
@berickson1 Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day! |
Co-authored-by: Ernesto García <gnapse@gmail.com>
060075e
to
30da26c
Compare
@gnapse, @jgoz, @majesticuser, @orta Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
@jgoz, @majesticuser, @orta Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
Ready to merge |
…s for toHaveDisplayValue by @berickson1 * [@testing-library/jest-dom] Add types for toHaveDisplayValue * Update as per PR comments * Update incorrect comment * Auto-commit github suggestion Co-authored-by: Ernesto García <gnapse@gmail.com> * Add tests from PR suggestions * Fix compiler errors from 3df262 Co-authored-by: Ernesto García <gnapse@gmail.com>
Fixes #44361
Please fill in this template.
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).Select one of these and delete the others:
If changing an existing definition:
tslint.json
containing{ "extends": "dtslint/dt.json" }
. If for reason the any rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]
and not for whole package so that the need for disabling can be reviewed.