-
Notifications
You must be signed in to change notification settings - Fork 43
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 fallback for window width #45
Conversation
@maxrimue I'll work on landing this this weekend, managed to lose track of 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.
looking good, I think it would be worth adding a safety for non-node environments.
@@ -310,7 +310,7 @@ module.exports = function (opts) { | |||
opts = opts || {} | |||
|
|||
return new UI({ | |||
width: (opts || {}).width || 80, | |||
width: (opts || {}).width || process.stdout.columns || 80, |
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 think some folks run yargs in the browser, so process
/process.stdout
might not exist. Let's pull this into a helper method that looks something like this:
function defaultWidth () {
width = null
if (process && process.stdout && process.stdout.columns) width = process.stdout.columns
return width
}
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.
@bcoe Agree, done
50b8269
to
4b16b35
Compare
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.
one tiny nit, otherwise I approve of this 👍
@@ -282,6 +282,10 @@ function _minWidth (col) { | |||
return minWidth | |||
} | |||
|
|||
function getWindowWidth () { | |||
if (process && process.stdout && process.stdout.columns) return process.stdout.columns |
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.
super small nit, let's go with typeof process === 'object'
, rather than just checking if process
is truthy. Otherwise, this is looking great.
looks good to me 👍 |
This adds a little fallback which returns the window width of the current window when no width is passed using the API.
If this PR gets merged, we could just not pass any width to cliui in yargs instead of requiring the rather expensive
window-size
.