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

638 Feature Request: password generator with options #664

Conversation

Christian-Oleson
Copy link

@Christian-Oleson Christian-Oleson commented Sep 19, 2022

Pull request type

Feature Request

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • UI change (please include screenshot!)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Internationalization and localization
  • Other (please describe):

What is the current behavior?

N/a

Issue Number: 638

What is the new behavior?

A new password generator feature has been made. You can select

  • Password Length
  • To include lowercase characters
  • To include uppercase characters
  • To include special characters
  • How many passwords to generate

Other information

  • My first PR to this repo
  • I am looking for initial feedback but may want to add Spanish and German translations to start, though I am unsure if you need all translations to be added up front.

Quality check

Before creating this PR, have you:

  • Followed the code style guideline as described in CONTRIBUTING.md
  • Verified that the change work in Release build configuration
  • Checked all unit tests pass

638 - Another...
@veler veler linked an issue Sep 19, 2022 that may be closed by this pull request
@veler
Copy link
Collaborator

veler commented Sep 19, 2022

Hello,
Thank you for this PR :D It looks great overall.

To give you some initial feedback and answer your questions:

  1. No need to translate the tool yet. We use Crowdin (https://crowdin.com/project/devtoys), which will automatically pull the added strings from en-us in the main branch. That means we should first approve and merge this PR before doing the translation.
  2. Since this is a new tool, we'd need to create a new icon for it. If you're willing to do it, you can try to follow the few instructions I left here: https://github.com/veler/DevToys/blob/main/assets/font/README.md
    Initially, these instructions were for me, as I'm not a pro a making icons and fonts. If you don't feel like doing the icon, let me know and I will happily do it.
    An idea of icon: how about something with *** or
    image
    This icon comes from FluentSystemIcons-Regular (glyph F59E) which we could copy and/or edit since it's under MIT license.

src/dev/DevToys.Startup/Package.appxmanifest Outdated Show resolved Hide resolved
src/dev/impl/DevToys/DevToys.csproj Outdated Show resolved Hide resolved
src/dev/impl/DevToys/Helpers/CryptoRandomGenerator.cs Outdated Show resolved Hide resolved
src/dev/impl/DevToys/Helpers/CryptoRandomGenerator.cs Outdated Show resolved Hide resolved
src/dev/impl/DevToys/Strings/en-US/PasswordGenerator.resw Outdated Show resolved Hide resolved
/// <summary>
/// Whether the password should include numbers.
/// </summary>
private static readonly SettingDefinition<bool> Numbers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really used?

Copy link
Author

Choose a reason for hiding this comment

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

I think I originally meant to add the numbers, but then realize the feature request didn't have anything about whether or not to include an option of include/exclude numbers. Would we like to do so? Or should we omit them from the options?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, currently we have the possibility to generate password without:

  • lowercase letters
  • uppercase letters
  • symbols
    If we also remove numbers... well there's not much to generate then 😅

But I do see the value for generating a password without numbers too.
In term of UX, I'm quite undecided at the moment, but in short, assuming we have 4 options

  • lowercase letters
  • uppercase letters
  • symbols
  • numbers
    we should enforce at least 1 option, or display a message asking the user to enable at least 1 option in order to generate a password.

These options could be displayed as:

  • Toggle check box (like today)
  • List with multiple selection
  • List of check box
  • else? (I can't think of anything else)

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

It would be very easy to display a message in the output text if no options are selected. The other options would require more work, but also would be acceptable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we give a try at the displaying a message?
Perhaps you can use an InfoBar like here: https://github.com/veler/DevToys/blob/2e99e04d6f363a5bddf044afc5545266be43246c/src/dev/impl/DevToys/Views/Tools/Text/RegEx/RegExToolPage.xaml#L118-L123 But display a Warning or Information instead of Error?

Another option I see (but it might be more complex too) is that when it remains only 1 option checked, we disable the checked option (the option will be checked but user won't be able to uncheck it) until another option is checked.

Copy link
Author

Choose a reason for hiding this comment

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

I'll take a look at this. It just may be awhile since I've gotten a bit busy.

<ex:IsCompactOverlayModeTrigger TargetElement="{x:Bind}" />
</VisualState.StateTriggers>
<VisualState.Setters>
<Setter Target="LengthSetting.Visibility" Value="Collapsed" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great, but it doesn't hide the Configuration text.
I'd suggest to give a name to the StackPanel on line 42 and collapse the whole StackPanel on line 21 (and remove lines 22 to 24).

Copy link
Author

Choose a reason for hiding this comment

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

I believe I made the modification desired here.

src/dev/shared/SharedAssemblyInfo.cs Outdated Show resolved Hide resolved
@Christian-Oleson
Copy link
Author

Hello, Thank you for this PR :D It looks great overall.

To give you some initial feedback and answer your questions:

  1. No need to translate the tool yet. We use Crowdin (https://crowdin.com/project/devtoys), which will automatically pull the added strings from en-us in the main branch. That means we should first approve and merge this PR before doing the translation.
  2. Since this is a new tool, we'd need to create a new icon for it. If you're willing to do it, you can try to follow the few instructions I left here: https://github.com/veler/DevToys/blob/main/assets/font/README.md
    Initially, these instructions were for me, as I'm not a pro a making icons and fonts. If you don't feel like doing the icon, let me know and I will happily do it.
    An idea of icon: how about something with *** or
    image
    This icon comes from FluentSystemIcons-Regular (glyph F59E) which we could copy and/or edit since it's under MIT license.

I'll have to see if I can find another computer. FontForge is not liked by my machine it seems. I'll work to get it on another and see if I can follow the setup to generate a

@Christian-Oleson
Copy link
Author

FYI, I'm still working this, just went out on parental leave last week, so it will take a bit longer than expected.

@veler
Copy link
Collaborator

veler commented Oct 9, 2022

FYI, I'm still working this, just went out on parental leave last week, so it will take a bit longer than expected.

@Christian-Oleson no worries :) I can address the remaining feedback myself if you'd like too.
Also, congratulations for the newborn! 💯

@nj
Copy link

nj commented Oct 31, 2022

Might be good to extend further later on, perhaps with inspiration by https://randomkeygen.com/ (on GitHub at https://github.com/circlecell/randomkeygen.com, MIT licensed).

@Christian-Oleson
Copy link
Author

FYI, I'm still working this, just went out on parental leave last week, so it will take a bit longer than expected.

@Christian-Oleson no worries :) I can address the remaining feedback myself if you'd like too.

Also, congratulations for the newborn! 💯

Thank you! It may be best to have you finish. I'm still out and a bit busy, so it may be best to get this released instead of having folks wait on me eventually completing this on my end.

@veler
Copy link
Collaborator

veler commented Jul 20, 2023

Closing this PR as the tool got completed into #873 . Thank you @Christian-Oleson and @micahmo for this contribution! :)

@veler veler closed this Jul 20, 2023
@Christian-Oleson Christian-Oleson deleted the 638-feature-request-password-generator-w branch July 20, 2023 15:30
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.

Feature Request: password generator with options
3 participants