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

Improve password generator #703

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

kcinay055679
Copy link
Collaborator

@kcinay055679 kcinay055679 commented May 22, 2023

No description provided.

@kcinay055679
Copy link
Collaborator Author

kcinay055679 commented May 24, 2023

Functional everything is fine.

TODO:

  • Design
  • Tests

@njaeggi njaeggi force-pushed the feature/687/password-generator-options branch from 1d378af to 64d1496 Compare May 25, 2023 08:19
@njaeggi njaeggi marked this pull request as ready for review May 25, 2023 09:11
@njaeggi njaeggi requested a review from Robin481 May 25, 2023 09:21
Copy link
Member

@Robin481 Robin481 left a comment

Choose a reason for hiding this comment

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

Looking quite nice.
However, I don't think the interface makes a lot of sense at this point.
Currently I would argue 95% of the time a user doesn't create a password in Cryptopus but rather saves an already existing password.

Therefore, the use case of going to an encryptable which already has a password and randomly generating a new one that then automatically overwrites the old one doesn't really exist in my opinion and is rather confusing.

Maybe a easy solution would be to not automatically overwrite the existing password but rather have a button that says "create new secure password" instead of always overwriting when using the slider or checking the symbol box.

frontend/app/components/encryptable/form.js Outdated Show resolved Hide resolved
frontend/app/styles/app.scss Show resolved Hide resolved
@njaeggi njaeggi requested a review from Robin481 May 25, 2023 12:52
Copy link
Member

@Robin481 Robin481 left a comment

Choose a reason for hiding this comment

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

Looking quite nice.
However the button takes up too mache space now.
What about putting it there and making it smaller? Think that would be the best of both worlds.
image

@njaeggi njaeggi requested a review from Robin481 May 26, 2023 08:34
@kcinay055679 kcinay055679 force-pushed the feature/687/password-generator-options branch from 6808b8f to 7a6fb3f Compare May 30, 2023 05:51
@kcinay055679 kcinay055679 linked an issue May 30, 2023 that may be closed by this pull request
4 tasks
@kcinay055679 kcinay055679 force-pushed the feature/687/password-generator-options branch from 00f352d to be6af01 Compare May 30, 2023 12:11
Copy link
Member

@Robin481 Robin481 left a comment

Choose a reason for hiding this comment

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

Looks very nice!
As discussed with @kcinay055679 a nice improvement would be to maybe not show the password generator when the field is already pre-filled. Work out what you think is a nice idea and let's discuss it :)

@kcinay055679
Copy link
Collaborator Author

I added a accordion to the password generator witch is expanded on default and auto collapses after generating a new password. But the Accordeon generated by my code isnt the same as the one from the docs. I think the reason for that behavior is that we are currently using bootstrap 4.6.

@Robin481
Copy link
Member

Robin481 commented Jun 1, 2023

@kcinay055679
Good start.
Make the dropdown smaller, give it a bottom margin and add an arrow to the dropdown that actually indicates that this is a drodopwn, something like this.
image
(and fix the styling errors)

@kcinay055679
Copy link
Collaborator Author

The requested arrow is default functionality according to the ember docs, but in our project the arrow is simpy not there.
i think this is because we're using bootstrap 4

@mtnstar mtnstar force-pushed the master branch 4 times, most recently from ecd3f3e to d610931 Compare June 21, 2023 14:08
@mtnstar mtnstar force-pushed the master branch 2 times, most recently from 520ebe8 to 151e944 Compare June 28, 2023 14:05
@kcinay055679 kcinay055679 force-pushed the feature/687/password-generator-options branch from 34a25ee to 07a87d2 Compare July 26, 2023 09:04
@kcinay055679 kcinay055679 marked this pull request as draft July 28, 2023 10:01
@kcinay055679 kcinay055679 marked this pull request as ready for review July 28, 2023 10:49
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.

More Password Generator options
4 participants