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

Creates a window to show multiple credentials for HTTP authentication #782

Closed

Conversation

furious
Copy link

@furious furious commented Feb 22, 2020

I started using KeePassXC but was missing a feature from the Kee Vault extension, so I implemented it here.

This PR creates a popup window for multiple credentials on HTTP basic authentication requests, its more convenient than clicking on the extension icon to select one (which will still be there).

I've tested it a lot and I think is working fine, let me know if you find any problems.

@droidmonkey
Copy link
Member

Screenshot?

@furious
Copy link
Author

furious commented Feb 22, 2020

Oops, forgot to upload a screenshot. It is centralized just like the native dialog.
image

@droidmonkey
Copy link
Member

Great idea!

@varjolintu
Copy link
Member

Thanks for the PR. This makes lot easier to get rid of some extra popup code in the future.

One thing I'm not sure of: now the dialog is centered by the screen. Should it be centered to the browser window instead?

@furious
Copy link
Author

furious commented Feb 23, 2020

One thing I'm not sure of: now the dialog is centered by the screen. Should it be centered to the browser window instead?

Good ideia, since the native dialog does that...

@furious
Copy link
Author

furious commented Feb 24, 2020

Here's how it looks now when you have more than 4 credentials

image

@varjolintu
Copy link
Member

@furious That dialog is way too small. It should show at least 8-10 credentials, just to make sure. Is there any way to resize it dynamically?

…tion, also better UI if have a lot of credentials
@furious
Copy link
Author

furious commented Feb 24, 2020

@varjolintu I don't know a good way to calculate and dynamically resize the dialog, any suggestions?

@varjolintu
Copy link
Member

varjolintu commented Feb 25, 2020

Well. If it's possible to know the height of one item on list before showing, it would make things easier, and you could adjust the window accordingly.

EDIT: Or just remove the lines you added to popup.css which makes the window scrollable instead.

@furious furious closed this Feb 25, 2020
@varjolintu
Copy link
Member

Accidental close?

@varjolintu varjolintu reopened this Feb 25, 2020
@furious
Copy link
Author

furious commented Feb 26, 2020

Well, this is probably my last effort for this pull request, I hope it meets the last requirements to be approved/merged.

@varjolintu
Copy link
Member

@furious Thanks for the update. Now it works great!

keepassxc-browser/background/event.js Outdated Show resolved Hide resolved
@furious
Copy link
Author

furious commented Jun 21, 2020

What is preventing this PR to be finally merged into develop branch?

@varjolintu varjolintu added this to the 1.6.5 milestone Jun 21, 2020
@varjolintu
Copy link
Member

@furious My laziness. I'll review this again to see everything's fine, and let's put this to the next release.

@varjolintu varjolintu self-requested a review July 6, 2020 13:22
@varjolintu
Copy link
Member

Btw, the dialog doesn't disappear if user decides to "cancel" the process and just to go another page. Would it be easy to fix?
Steps:

  • Go to HTTP Basic Auth page.
  • Dialog appears.
  • Just click the browser URL field and go to another page.

@varjolintu varjolintu modified the milestones: 1.6.5, 1.6.6, 1.6.7 Jul 11, 2020
@ThinkChaos
Copy link

Btw, the dialog doesn't disappear if user decides to "cancel" the process and just to go another page. Would it be easy to fix?
Steps:

* Go to HTTP Basic Auth page.

* Dialog appears.

* Just click the browser URL field and go to another page.

Any chance on getting this merged and fixing this separately?
This is a huge UX gain.

@varjolintu
Copy link
Member

I had big troubles trying to rebase this PR. Lets continue at: #1064

@varjolintu varjolintu closed this Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants