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

Add CTRL+Enter to apply password generator changes #6414

Merged
merged 2 commits into from
Apr 19, 2021

Conversation

xvallspl
Copy link
Contributor

@xvallspl xvallspl commented Apr 15, 2021

add the shortcut CTRL+Enter as an option (on top of the current CTRL+S)
to close the Password Generator widget and apply the changes.

Closes #6111

Assuming desired backwards compatibility and keeping the CTRL+S shortcut

Checklist:

  • Implement functionality
  • Document functionality

Testing strategy

How can I test this? @droidmonkey @phoerious

Type of change

  • ✅ New feature (change that adds functionality)
  • ✅ Documentation (non-code change)

add the shortcut CTRL+Enter as an option (on top of the current CTRL+S)
to close the Password Generator widget and apply the changes.
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

I wonder if the QShortcuts should be parented to the button instead of the generator widget. I don't really know how Qt handles accessibility features when there is no direct connection between the button and the shortcut.

@phoerious phoerious added the ux label Apr 15, 2021
@phoerious phoerious added this to the v2.7.0 milestone Apr 15, 2021
@droidmonkey
Copy link
Member

Which accessibility would this impact? Keyboard shortcuts are hidden in the UI besides actions in menus, you need to explicitly write about them to expose to end users.

@xvallspl
Copy link
Contributor Author

For the documentation, I am updating both subsections in the Password Generator section in the User Guide with an extra step, but I am referencing a button that appears in the app but not in the screenshot.

My issue is that I don't have access to a linux box and the screenshots seem to be taken on linux. Can someone help me with this for consistency's sake?

@xvallspl
Copy link
Contributor Author

I wonder if the QShortcuts should be parented to the button instead of the generator widget. I don't really know how Qt handles accessibility features when there is no direct connection between the button and the shortcut.

FWIW I was not sure I could find a way to connect several shortcuts with the button, so I connected them to the action.

@xvallspl xvallspl marked this pull request as ready for review April 16, 2021 05:53
@droidmonkey
Copy link
Member

droidmonkey commented Apr 16, 2021

The screenshot in the documentation is the "stand alone" password generator that doesn't have the apply button. For the docs, it would be good enough to just say "When generating a password for an entry, press ...." no need for another screenshot. (we try to limit those cause it bloats the size of the docs)

As for screenshots in documentation, I took those in windows using the light theme. I also run them through https://tinypng.com compression which I found to be the best. I can take any screenshots you want to be extra consistent.

@droidmonkey
Copy link
Member

droidmonkey commented Apr 16, 2021

Merging tip: if you merge this yourself now that it's approved, always do a squash merge to get it down to one commit and rebased onto develop. If it's already one commit do a rebase merge. In very rare cases we use a merge commit.

Although looks like I need to fix the macos CI again...

@xvallspl xvallspl force-pushed the feature/ctrl-Enter-password-generator branch from 4ab1d40 to 3617079 Compare April 17, 2021 17:13
@xvallspl
Copy link
Contributor Author

Updated the documentation.

@droidmonkey I don't have merging permissions :)

@droidmonkey droidmonkey linked an issue Apr 19, 2021 that may be closed by this pull request
@droidmonkey droidmonkey changed the title allow CTRL+Enter to apply password generator changes. Fixes #6111. Add CTRL+Enter to apply password generator changes Apr 19, 2021
@droidmonkey droidmonkey merged commit c0ae130 into develop Apr 19, 2021
@droidmonkey droidmonkey deleted the feature/ctrl-Enter-password-generator branch April 19, 2021 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Ctrl+Enter to confirm password generator
3 participants