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

Fix #1298: Text input support on AlertPopupView #1341

Merged
merged 1 commit into from Aug 8, 2019
Merged

Conversation

@jamesmudgett
Copy link
Contributor

jamesmudgett commented Aug 7, 2019

Adds text input support for AlertPopupView. Supports all input(keyboard) types and secure input as well. Example usage:

let popup = AlertPopupView(image: #imageLiteral(resourceName: "browser_lock_popup"), title: "PIN verification required", message: "", inputType: .asciiCapable, secureInput: true, inputPlaceholder: "Enter your Yubikey PIN")

        popup.addButton(title: "Cancel") { () -> PopupViewDismissType in
            return .flyDown
        }

        popup.addButton(title: "Verify", type: .primary) { [weak self] () -> PopupViewDismissType in
            guard let pin = popup.text else { return .deny }

            // ...more error handling.
            if pin.isEmpty {
                return .deny
            }

            print(pin)

            return .flyUp
        }
        browserLockPopup = popup
        popup.showWithType(showType: .flyUp)

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • My patch or PR title has a standard commit message that looks like Fix #123: This fixes the shattered coffee cup! (or No Bug: <message> if no relevant ticket)
  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()
  • New files have MPL-2.0 license header.

Test Plan:

Screenshots:

Simulator Screen Shot - iPhone Xs - 2019-08-07 at 15 56 41

Reviewer Checklist:

  • PR is linked to an issue via Zenhub.
  • Issues are assigned to at least one epic.
  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable)
@jamesmudgett jamesmudgett requested review from Brandon-T, iccub and jumde and removed request for Brandon-T Aug 7, 2019
Copy link
Contributor

iccub left a comment

Few code style comments

@@ -48,6 +52,22 @@ class AlertPopupView: PopupView {
messageLabel.numberOfLines = 0
containerView.addSubview(messageLabel)

if let inputType = inputType {

This comment has been minimized.

Copy link
@iccub

iccub Aug 7, 2019

Contributor

add let textField = textField here to avoid unwrappings

This comment has been minimized.

Copy link
@jamesmudgett

jamesmudgett Aug 8, 2019

Author Contributor

Not needed here with change to use Then framework.

Client/Frontend/Popup/AlertPopupView.swift Outdated Show resolved Hide resolved
fileprivate var dialogImage: UIImageView?
fileprivate var titleLabel: UILabel!
fileprivate var messageLabel: UILabel!
fileprivate var containerView: UIView!
fileprivate var textField: UITextField?

public var text: String? {

This comment has been minimized.

Copy link
@iccub

iccub Aug 7, 2019

Contributor

Remove public, it's needed only in frameworks

Client/Frontend/Popup/AlertPopupView.swift Outdated Show resolved Hide resolved
@@ -104,10 +104,22 @@
"version": "github:brave/ad-block#b933cf5168c5284dc159735d8bcefb415d0cfc00",
"from": "github:brave/ad-block#b933cf5168c5284dc159735d8bcefb415d0cfc00",
"requires": {
"bloom-filter-cpp": "^1.2.2",
"bloom-filter-cpp": "^1.2.0",

This comment has been minimized.

Copy link
@iccub

iccub Aug 7, 2019

Contributor

It would be best to not version this package-lock.json file at all for this PR

Client/U2FExtensions.swift Outdated Show resolved Hide resolved
Client/Frontend/Popup/AlertPopupView.swift Outdated Show resolved Hide resolved
Client/Frontend/Popup/AlertPopupView.swift Outdated Show resolved Hide resolved
Client/U2FExtensions.swift Outdated Show resolved Hide resolved
@jamesmudgett jamesmudgett force-pushed the 2fa_input_popup branch from 8b8c0b2 to ebd801e Aug 8, 2019
@jamesmudgett jamesmudgett requested review from jhreis and iccub Aug 8, 2019
@jhreis
jhreis approved these changes Aug 8, 2019
@jhreis
Copy link
Contributor

jhreis commented Aug 8, 2019

lgtm. @jamesmudgett in future please make sure to complete the PR template submitter checklist.

@iccub feel free to merge if you approve it, please just verify the squashed message, as it may not be correct by default.

@jhreis jhreis changed the title Text input support on AlertPopupView Fix #1298: Text input support on AlertPopupView Aug 8, 2019
@iccub
iccub approved these changes Aug 8, 2019
@iccub iccub merged commit 204da56 into development Aug 8, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@iccub iccub deleted the 2fa_input_popup branch Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.