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 missing lines in the ios picker #13342

Closed
wants to merge 1 commit into from

Conversation

pvinis
Copy link
Contributor

@pvinis pvinis commented Apr 6, 2017

fix missing lines in the ios picker. thanks to http://stackoverflow.com/questions/39564660/uipickerview-selection-indicator-not-visible-in-ios10

Motivation (required)

as written in http://stackoverflow.com/questions/39564660/uipickerview-selection-indicator-not-visible-in-ios10 there is a case where the picker is missing the highlight lines for the currently selected picker item. this fixes this case.

Test Plan (required)

no tests needed other than the highlight lines appear like in the stackoverflow link. and it works every time now, instead of sometimes lines missing.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Apr 6, 2017
@hramos
Copy link
Contributor

hramos commented Apr 27, 2017

The test plan seems incomplete. See https://medium.com/@martinkonicek/what-is-a-test-plan-8bfc840ec171 for more details.

@@ -19,7 +19,9 @@ @implementation RCTPickerManager

- (UIView *)view
{
return [RCTPicker new];
RCTPicker *picker = [RCTPicker new];
[picker selectRow:0 inComponent:0 animated:YES];
Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely should not configure it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whats different than before? we are not configuring anything here, im just setting some defaults so the lines appear.

Copy link
Contributor

Choose a reason for hiding this comment

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

It just should be inside RCTPicker's init method, and View Managers should be as declarative as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pvinis can you address @shergin's feedback?

Copy link

@adnanbabar adnanbabar Mar 1, 2018

Choose a reason for hiding this comment

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

The fix works fine within React/Views/RCTPickerManager.m but when I move it within RCTPicker's initWithFrame it doesn't work.
@shergin what am I doing wrong?

Thanks

screen shot 2018-03-01 at 13 39 10

@shergin shergin added the Platform: iOS iOS applications. label May 31, 2017
@pvinis pvinis closed this Jul 19, 2017
@pvinis pvinis deleted the fix/ios-picker-missing-lines branch July 19, 2017 09:01
@pvinis
Copy link
Contributor Author

pvinis commented Jul 24, 2017

maybe we should reopen this? #14442 is fixed with this pr.
what is blocking this to merge?

@hramos
Copy link
Contributor

hramos commented Jul 24, 2017

It looks like back in April I had some concerns with the test plan. After looking at it again, I do feel like the test plan is incomplete.

@pvinis
Copy link
Contributor Author

pvinis commented Jul 24, 2017

i actually added a couple of test views in the RNTester, but i couldnt make the bug happen.
would that be enough of a test plan?

@hramos
Copy link
Contributor

hramos commented Jul 24, 2017

Looks like I can't re-open the PR because the source branch was deleted.

@shergin
Copy link
Contributor

shergin commented Jul 24, 2017

@pvinis Just put the hack inside initWithFrame: method and add detailed comment with the link to SO. And we good to go!

@pvinis
Copy link
Contributor Author

pvinis commented Jul 24, 2017

i will try to make the bug happen again and make the branch again, plus the test views.

@shergin unfortunatelly, still better than many other PRs..

@adnanbabar
Copy link

Hi,

Is there any update on this please I am trying to use picker and the lines disappear when I interact with it...
Thanks

@hramos
Copy link
Contributor

hramos commented Feb 28, 2018

No update at all - the pull request is closed and the source branch long gone.

@adnanbabar
Copy link

@hramos

Thanks for the quick reply. But this is still an issue, any plan for fixing this in future, please?

Also, let me know if I can help in any way...

@hramos
Copy link
Contributor

hramos commented Feb 28, 2018

Sure, a good approach right now is to open a proper issue here in the repo with all the pertaining information. Even better - pick up on the work started here and submit your own PR.

@VSchlattinger
Copy link
Contributor

@adnanbabar I created a new PR for this issue, where I moved the code to the initWithFrame function in React/Views/RCTPicker.m and it still fixed the problem: #18885

Hopefully that one will be merged.

@pvinis
Copy link
Contributor Author

pvinis commented Apr 16, 2018

i hope so too. thanks @VSchlattinger.

facebook-github-bot pushed a commit that referenced this pull request Jul 21, 2018
Summary:
This PR is based on #13342 by pvinis and fixes #14442.

As suggested in the discussion on the PR mentioned above, I moved the code from `React/Views/RCTPickerManager.m` to the `initWithFrame` function in `React/Views/RCTPicker.m` and verified that it still fixes the problem.

Before the change in this PR the selection indicator lines are missing when the Picker is initially added to the View and only appear after changing to a different Tab and back. This happens both in the Simulator and my real device (iPhone 6S on iOS 11.3).

![beforechange](https://user-images.githubusercontent.com/7568362/38824104-7b294cae-41a8-11e8-8609-7a647ab3adb8.png)

After the change, the indicator lines always appear. I did not notice any side effects of this change when playing around with the Picker in different configurations.

![afterchange](https://user-images.githubusercontent.com/7568362/38824109-82a5b382-41a8-11e8-8af3-ca07c8b2c30e.png)

#13342 also fixes this issue but appears to be inactive.

[IOS] [BUGFIX] [PickerIOS] - Fixed missing selection indicator lines
Pull Request resolved: #18885

Differential Revision: D8945483

Pulled By: hramos

fbshipit-source-id: 2b6c6f42559691530b062503feb24bc305f4a8af
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants