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

CertWizard: Password requirement notice on import #2800

Merged
merged 1 commit into from Feb 1, 2017

Conversation

@Kissaki
Copy link
Member

commented Feb 1, 2017

When a (p12) certificate file is specified, it is being imported. If this
is not successful it is either because of a missing or wrong password, or
because the file is in fact otherwise not valid.

Instead of simply disabling and enabling the wizard Next button we now
display a text notice to the user to make this clear (especially for the
common use-case of having to provide a correct password).

Fixes #1025


nopw
pw-req
pw-suc

CertWizard: Password requirement notice on import
When a (p12) certificate file is specified, it is being imported. If this
is not successful it is either because of a missing or wrong password, or
because the file is in fact otherwise not valid.

Instead of simply disabling and enabling the wizard Next button we now
display a text notice to the user to make this clear (especially for the
common use-case of having to provide a correct password).

Fixes #1025

@Kissaki Kissaki added the ui label Feb 1, 2017

@Kissaki Kissaki requested review from mkrautz and hacst Feb 1, 2017

@mkrautz

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

What does it mean when the password text input field is disabled? I know that's not something you added in this PR, but that seems very confusing to me.

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2017

Yeah, I could improve on that as well - here or separately.

The password field is being disabled when no cert is selected or the cert was read successfully. So it will be disabled when you enter the correct password as well (which is … interesting).

@mkrautz

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

Nah, we can do it in multiple steps. This does seem like a nice improvement already.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

The code looks fine to me, but I worry about the color WRT accessibility/color-blindness. Since there is only an error state, I don't think it should be a big problem. But perhaps we can do better?

LGTM for the easy fix, though.

@mkrautz
mkrautz approved these changes Feb 1, 2017
@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2017

The text is in italic. That should make it sufficiently distinguishable. Do we do anything else elsewhere?

@mkrautz

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

No, it's not like we do it any better elsewhere. I'm OK with this.

@Kissaki Kissaki merged commit 8f94c76 into mumble-voip:master Feb 1, 2017

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2017

Maybe the cert path should not be editable, and the browser for certificate should ask for a password via dialog before returning to the page. I think that would be better. Maybe I'll come back to it.

@Kissaki Kissaki deleted the Kissaki:cert-pw branch Feb 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.