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.

Sign up for GitHub

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

Jump to bottom

df/Issue115 #126

Merged
merged 42 commits into from
Aug 26, 2022
Merged

df/Issue115 #126

merged 42 commits into from
Aug 26, 2022

Conversation

dfeinzimer
Copy link
Collaborator

Closes #115

@dfeinzimer dfeinzimer self-assigned this Aug 17, 2022
@dfeinzimer dfeinzimer linked an issue Aug 17, 2022 that may be closed by this pull request
Display user/password & cert password prompts as an alert #115
Closed
@dfeinzimer dfeinzimer marked this pull request as ready for review August 18, 2022 00:00
@rolson
Copy link
Contributor

rolson commented Aug 18, 2022

I think the text in these alerts is too bold/big.

image

image

If it can be broken into a title/message it would look something more like this and I think appear a bit more natural:

image

I think the current text in the alerts make good "description" or smaller text. There could be a title for each of them. Maybe for the PKI certificate password alert it would be "Password Required". And for the username/password alert it could be "Authentication Required".

yo1995
yo1995 reviewed Aug 18, 2022
View reviewed changes
Sources/ArcGISToolkit/Components/Authentication/CertificatePickerViewModifier.swift Outdated
/// - Parameter textField: The text field who's value recently changed.
func updateValues(with textField: UITextField) {
parent.viewModel.password = textField.text ?? ""
parent.continueAction.isEnabled = !parent.viewModel.password.isEmpty
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an observation on UITextField.textDidChangeNotification instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had considered that but it seemed to add additional complexity because I believe it would've required an ObjC selector and ObjC methods aren't allowed in structs. However, as of this morning I discovered the newer addAction(_:for:) method and so this is now removed.

@dfeinzimer dfeinzimer requested a review from rolson August 18, 2022 19:39
@njarecha njarecha mentioned this pull request Aug 19, 2022
Merged
@dfeinzimer dfeinzimer mentioned this pull request Aug 22, 2022
Merged
dfeinzimer and others added 16 commits August 23, 2022 09:34
mhdostal
mhdostal reviewed Aug 24, 2022
View reviewed changes
Copy link
Member

@mhdostal mhdostal left a comment

Choose a reason for hiding this comment

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

I've added my comments to the follow-on PR.

@dfeinzimer dfeinzimer merged commit 69a6229 into v.next Aug 26, 2022
@dfeinzimer dfeinzimer deleted the df/Issue115 branch August 26, 2022 14:58
@dfeinzimer dfeinzimer mentioned this pull request Nov 3, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yo1995 yo1995 yo1995 left review comments

@mhdostal mhdostal mhdostal approved these changes

@rolson rolson rolson approved these changes

Assignees

@dfeinzimer dfeinzimer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Display user/password & cert password prompts as an alert
4 participants
@dfeinzimer @rolson @mhdostal @yo1995