-
Notifications
You must be signed in to change notification settings - Fork 471
feat: add a return value from logTestingPlaygroundURL #1144
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
feat: add a return value from logTestingPlaygroundURL #1144
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4bcb210:
|
docs for this PR testing-library/testing-library-docs#1105 |
Codecov Report
@@ Coverage Diff @@
## main #1144 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 987 989 +2
Branches 325 325
=========================================
+ Hits 987 989 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
@timdeschryver can you please approve? 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.
LGTM, leaving it to @MatanBobi
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 @Lirlev48 for the contribution :) Left a small comment.
@timdeschryver thanks for reviewing this one :)
Do you see any issue with this? I'm not sure if this change can somehow break something, wdyt?
src/screen.ts
Outdated
@@ -42,6 +42,7 @@ const logTestingPlaygroundURL = (element = getDocument().body) => { | |||
console.log( | |||
`Open this URL in your browser\n\n${getPlaygroundUrl(element.innerHTML)}`, | |||
) | |||
return getPlaygroundUrl(element.innerHTML) |
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.
Small nit, can we save the `getPlaygroundUrl in a const instead of calling the function twice? Not sure what the performance aspects for it, but better safe than sorry :)
I think this is safe @MatanBobi . |
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 @Lirlev48 :)
@all-contributors please add @Lirlev48 for code :) |
I've put up a pull request to add @Lirlev48! 🎉 |
🎉 This PR is included in version 8.16.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Adding a return value to logTestingPlaygroundURL function besides the log - Resolves #1125
logTestingPlaygroundURL() used to prints the value directly to the stdout of the running process,
The values was hidden in the log window
Adding a return statement to the logTestingPlaygroundURL function
Checklist:
docs site
Will add documentation to testing-library-docs after the PR will be completed.