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

Feat (password): add option to toggle password visibility #1334

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matteosacchetto
Copy link
Contributor

@matteosacchetto matteosacchetto commented Nov 19, 2023

Implementation of the feature request idea #1330

Steps:

  • Add basic functionality to skip the transform function
  • Add option to show or hide the password
  • Add tests
  • Add documentation
  • Decide the shortcut to use
  • Decide how to manage the UX

@matteosacchetto matteosacchetto marked this pull request as draft November 19, 2023 15:42
Copy link

codecov bot commented Nov 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (11bbb4b) 94.48% compared to head (ae62463) 94.54%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1334      +/-   ##
==========================================
+ Coverage   94.48%   94.54%   +0.06%     
==========================================
  Files          51       51              
  Lines        4457     4476      +19     
  Branches      775      780       +5     
==========================================
+ Hits         4211     4232      +21     
+ Misses        241      239       -2     
  Partials        5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matteosacchetto
Copy link
Contributor Author

I started implementing the feature request of #1330
I still have to add tests, and handle the UX

Moreover, we also need to decide how we should call the new options

@matteosacchetto
Copy link
Contributor Author

@SBoudrias is there a way to test keyboard combinations, like CTRL + space?

@SBoudrias
Copy link
Owner

is there a way to test keyboard combinations, like CTRL + space?

Not at the moment, we should definitively add it to the testing library though!

@matteosacchetto
Copy link
Contributor Author

is there a way to test keyboard combinations, like CTRL + space?

Not at the moment, we should definitively add it to the testing library though!

I will look into this further, since I think is not that difficult to add to the library, and it would allow us to write tests for this new feature

@matteosacchetto
Copy link
Contributor Author

matteosacchetto commented Nov 22, 2023

is there a way to test keyboard combinations, like CTRL + space?

Not at the moment, we should definitively add it to the testing library though!

I will look into this further, since I think is not that difficult to add to the library, and it would allow us to write tests for this new feature

Quick update: to allow for testing keyboard combinations is relatively easy to implement. We just need to add to @inquirer/testing an additional event, which exposes all the parameters a 'keypress' event supports

export interface Key {
  name?: string | undefined;
  ctrl?: boolean | undefined;
  meta?: boolean | undefined;
  shift?: boolean | undefined;
}

We just need to decide whether to implement a new function, modify the current keypress without a breaking change or modify it with a breaking change.
I would say it would be better to implement a new function or to modify the keypress function without introducing a breaking change, since the option of passing keyboard combinations/shortcuts is less used than passing normal key presses

@matteosacchetto matteosacchetto changed the title Feat (input,password): add option to skip the transformer function Feat (password): add option to toggle password visibility Nov 23, 2023
Comment on lines 90 to 96
if (togglePassword && status !== 'done') {
formattedValue = value;
}

export default password;
if (status === 'done') {
formattedValue = chalk.cyan(formattedValue);
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think there's a small logic mistake here. When the prompt is completed, we shouldn't show the password in plain text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the prompt is completed, status will be equal to 'done', so the if at line 90 will be false, and formattedValue will remain equal to the transformed value. In the tests, I explicitly check for this scenario, in order to avoid password to be shown in plain text after the prompt is completed.

Comment on lines +56 to +53
// CTRL + Space
// I only tried on Linux, but the combination on Linux was reported like that
// key.crtl = true
// key.name = '`'
Copy link
Owner

Choose a reason for hiding this comment

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

Sooo, did some more digging into this. And it turned out to be a bit of a headache.

Depending on the environment and the terminal used, there can be a lot of conflict.

For one, CTRL + Space for me open the AI helper sidebar and never reach the node program.

I then started to think of keys; like CTRL+H, but that's a backspace shortcut (and it doesn't even register as the H key.)

So long story short:

  1. I added node tools/keys.mjs to the repo latest version to play with.
  2. Potential solution 1: finding a key combo that'll reasonably work for most environment (terminals, OS & consider non-english keyboards).
  3. Have a basic OS switch and use keys per OS that are reasonably going to work.

When the user presses the CTRL+Space combination, transformer is toggled
@SBoudrias SBoudrias force-pushed the feat-skip-transform branch from eeb543d to ae62463 Compare January 27, 2024 19:13
@matteosacchetto
Copy link
Contributor Author

Separating the rewrite from the new feature was the right choice!

I'm still experimenting with ideas on how to implement this feature but I have not yet found something which works consistently between different terminals

I think that if we want to ship this feature, in the end we need to find a shortcut which works on most of them and accept that in some environments it won't work as expected

If only there was a way to get which keyboard shortcuts are available in a give environment...

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