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

Update InlinePickerRowFormer.swift - system version check #31

Closed
wants to merge 1 commit into from

Conversation

yaricom
Copy link

@yaricom yaricom commented Jun 2, 2016

When trying to compare current system version against lowest supported "8.0.0" you should use .OrderedDescending instead of .OrderedAscending, because system version must be higher or equal to the specified one.

When trying to compare current system version against lowest supported "8.0.0" you should use .OrderedDescending instead of .OrderedAscending, because system version must be higher or equal to the specified one.
@ra1028
Copy link
Owner

ra1028 commented Sep 4, 2016

See #30 plz.

@yaricom
Copy link
Author

yaricom commented Sep 5, 2016

Yes, due to error in system version check it works on iOS with version >= 8.0 not as expected.
I.e. $0.cell.pickerView.reloadAllComponents() is not invoked.

@mattjgalloway
Copy link
Contributor

Shouldn't it actually be != .OrderedAscending?

And yes, @yaricom is right, we need this.

@mattjgalloway
Copy link
Contributor

Note that the PR needs changing anyway because the enum changed with Swift 3. But same problem.

@mattjgalloway
Copy link
Contributor

Ah I see you removed this code now @ra1028. I believe that's wrong. The check was the wrong way round in the first place, so while you removed iOS 7 support, you thought you were doing the right thing, but actually the call needs to be there. reloadAllComponents was iOS 8+, so you were attempting to gate it out for iOS 7. But in face you were gating it in for iOS 7 before - which would have caused a crash.

Anyhow, we need the reloadAllComponents in there.

@ZacharyKhan
Copy link
Collaborator

Any updates on this? Still having the same issue over a year later 👎

@mattjgalloway
Copy link
Contributor

@ZacharyKhan I would suggest submitting a new PR with the reloadAllComponents added back in.

@ZacharyKhan
Copy link
Collaborator

ZacharyKhan commented Nov 27, 2017

@mattjgalloway Working on that as I type this 👍 Should be in within an hour. I've also made some other changes that will be noted.

EDIT: Submitted as PR #64

@ra1028
Copy link
Owner

ra1028 commented Nov 29, 2017

Thank you for PR! @ZacharyKhan
and thanks for the comment. @mattjgalloway

But I'm losing motivation to maintain the Former now.
So, may I invite to the Coraborators of Former repo?

@ZacharyKhan
Copy link
Collaborator

Feel free to invite me, I’d be happy to help out where I’m able.

@ra1028
Copy link
Owner

ra1028 commented Nov 29, 2017

@ZacharyKhan
Thanks!
I've sent you an invitation now.
Please go right ahead about to push the commits, merge PR and so on!

@ZacharyKhan
Copy link
Collaborator

Closing, #64 has been merged to master & this has conflicts with master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants