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

Orientation support #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Orientation support #45

wants to merge 2 commits into from

Conversation

bill350
Copy link

@bill350 bill350 commented Apr 12, 2017

This PR allows to specify if the lock screen can autorotate and which mask is allowed on it.

The use case: we want to lock the PasscodeLock screen in portrait and our app is rotating. The lock screen appear on a displayed landscape screen (the app is full portrait except one screen) and we want the lock screen on portrait.

This PR also fix the demo project with the TouchIDReason property which is missing in the PasscodeLockConfiguration with the PasscodeLockConfigurationType protocol.

@ziogaschr
Copy link
Collaborator

Very nice @bill350.

What if you add a PasscodeLockConfigurationType extension for setting the default values for backward combatibility?

public protocol PasscodeLockConfigurationType {
    var repository: PasscodeRepositoryType {get}
    var passcodeLength: Int {get}
    var isTouchIDAllowed: Bool {get set}
    var shouldRequestTouchIDImmediately: Bool {get}
    var touchIdReason: String? {get set}
    var maximumInccorectPasscodeAttempts: Int {get}
    var supportedInterfaceOrientations: UIInterfaceOrientationMask {get}
    var shouldAutorotate: Bool {get}
}

// set configuration optionals
extension PasscodeLockConfigurationType {
    var passcodeLength: Int {
        return 4
    }

    var maximumInccorectPasscodeAttempts: Int {
        return -1
    }
    
    var supportedInterfaceOrientations: UIInterfaceOrientationMask {
        return .portrait
    }
    
    var shouldAutorotate: Bool {
        return false
    }
}

Let me know what you think, and after that I will try to also test it in order we merge this.

@bill350
Copy link
Author

bill350 commented Jun 2, 2017

Thank you @ziogaschr
Yes I think its a good point to propose a default implementation. Go to test it with a default + a custom extension.

@ziogaschr
Copy link
Collaborator

ziogaschr commented Jun 7, 2017

Can you @bill350 add the default implementation before I can test this? Thanks

@bill350
Copy link
Author

bill350 commented Jun 9, 2017

@ziogaschr Done ;)

@ziogaschr
Copy link
Collaborator

Thanks @bill350, will try to check it as soon as possible.

@ziogaschr ziogaschr self-assigned this Jun 10, 2017
@bill350
Copy link
Author

bill350 commented Jul 23, 2017

Hey @ziogaschr, can we merge it ?

@ziogaschr
Copy link
Collaborator

Hi @bill350, sorry for taking me so long to test it.
I am migrating the code for Swift 4 and I gave a test to your branch too.

The code is good! 👍

Although, I think that we have to do something with the UI too, as it doesn't fit nicely on the screen.
Do you have any ideas? I was thinking that we might be able to move the keypad on the right, and the rest info and button on the left of the screen, but not sure without trying.

image

Are you able to pickup this?

Thanks, Chris.

@bill350
Copy link
Author

bill350 commented Sep 10, 2017

In a personal project I have created buttons inside a scrollView, and it's working fine. So we juste have to update the demo project ;-)

@bill350
Copy link
Author

bill350 commented Sep 10, 2017

I did this because of iPhone 4 & 5 screen limitation. I you agree to switch to a scrollView for the demo ?

@ziogaschr
Copy link
Collaborator

@bill350 scrollView sounds as a possible alternative, but I don't think it's optimal as the current elements can fit in screen without a scrollView. Also the above screenshot is from an iPhone6s and the UI is not fitting there too.

With that said, I am just a contributor like you, and I am not the only one to decide about it. So I am happy that you suggested this idea and that you are helping. 👍

Let's see what others believe too. :)

@bill350
Copy link
Author

bill350 commented Nov 5, 2017

Hey @ziogaschr
So after the Swift 4 migration and because of no more activity on this thread, do you want and updated Swift 4 version ? What's your new opinion since your last one ?
Thanks
🍻

@ziogaschr
Copy link
Collaborator

Hey @bill350, I really have no clue for what's best about the landscape UI. @velikanov or any other member any ideas?

If we don't find any ideas, I will try to test this better and merge it, later on, someone else might contribute with a better UI.

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.

2 participants