Skip to content
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

Use dots animation in certain Windows shells that support it #12

Merged

Conversation

MLoughry
Copy link
Contributor

Both the inegrated terminal in VSCode, as well as the new Windows Terminal (currently in preview) support the Unicode characters used in the dots animation.

This change refactors the platform check to a separate function, and checks environment variables set by VSCode ( microsoft/vscode#29426 ) and Windows Terminal ( microsoft/terminal#1040 )

Copy link
Owner

@jbcarpanelli jbcarpanelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for contributing @MLoughry (and sorry for the late response) 🎉! These Windows improvements are great, loved them!
I added some nit comments, could you please review them? After that we should be ready to merge!

changelog.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
utils.js Outdated Show resolved Hide resolved
utils.js Show resolved Hide resolved
utils.js Outdated
@@ -47,8 +47,8 @@ function colorOptions({ color, succeedColor, failColor, spinnerColor }) {
}

function prefixOptions({ succeedPrefix, failPrefix }) {
succeedPrefix = succeedPrefix ? succeedPrefix : (process.platform === 'win32' ? '√' : '✓');
failPrefix = failPrefix ? failPrefix : (process.platform === 'win32' ? '×' : '✖');
succeedPrefix = succeedPrefix ? succeedPrefix : (!terminalSupportsUnicode() ? '√' : '✓');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we can refactor this function a bit? Having nested ternary operators can be a bit confusing to understand
We can do something like:

if(terminalSupportUnicode()) {
  succeedPrefix = succeedPrefix || '✓';
  failPrefix = failPrefix || '✖';
} else {
  succeedPrefix = succeedPrefix || '√';
  failPrefix = failPrefix || '×';
}

I think it's easier to understand. It's not a blocker though 🙂.

MLoughry and others added 5 commits July 1, 2019 10:06
Co-Authored-By: Juan Bautista Carpanelli <[email protected]>
Co-Authored-By: Juan Bautista Carpanelli <[email protected]>
Co-Authored-By: Juan Bautista Carpanelli <[email protected]>
@jbcarpanelli jbcarpanelli merged commit 80fa603 into jbcarpanelli:master Jul 1, 2019
@jbcarpanelli
Copy link
Owner

I've just pushed a minor fix to your branch. To master it goes! thanks again for contributing! 🚢 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants