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 annoying Android and iOS issues #24626

Closed
3 of 7 tasks
cpojer opened this issue Apr 26, 2019 · 16 comments
Closed
3 of 7 tasks

☂️ Fix annoying Android and iOS issues #24626

cpojer opened this issue Apr 26, 2019 · 16 comments
Labels
Bug p: Facebook Partner: Facebook Platform: Android Android applications. Platform: iOS iOS applications. Ran Commands One of our bots successfully processed a command. Stale There has been a lack of activity on this issue and it may be closed soon.

Comments

@cpojer
Copy link
Contributor

cpojer commented Apr 26, 2019

🐛 Bug Report

Through react-native-community/discussions-and-proposals#104 and react-native-community/discussions-and-proposals#69 we learned about a lot of issues that people pointed out about React Native (cc @slorber @noahtallen @alburdette619). While we addressed many, there are still many issues remaining that need to be validated and if they still persist, fixed.

If you'd like to work on any of these, please comment here to sign up for one of them and send a Pull Request with a fix!

Thank you :)

@cpojer cpojer changed the title ☂️ Fix annoying Android issues ☂️ Fix annoying Android and iOS issues Apr 26, 2019
@react-native-bot react-native-bot added Platform: Android Android applications. Platform: iOS iOS applications. labels Apr 26, 2019
@react-native-bot

This comment has been minimized.

@react-native-bot react-native-bot added Ran Commands One of our bots successfully processed a command. Resolution: Needs More Information labels Apr 26, 2019
@guhungry
Copy link
Contributor

One of the toggle secureTextEntry issue(s) was addressed in #23524

@sasurau4
Copy link
Contributor

sasurau4 commented Apr 26, 2019

I want to work on the issue of style={{ width: "100" }} and forgetting the %.
I have a plan that fix the issue by translate style={{ width: "100"}} to style={{ width: "100%" }} automatically with warning rather than error.
Is the plan good? If not, please tell me other plans 🙏

@cpojer
Copy link
Contributor Author

cpojer commented Apr 26, 2019

@sasurau4: give it a try! I’ll wait for your pull request. I think just making sure the app doesn’t crash and shows a useful error message is a good start!

@matheusmonte
Copy link

Can work in "On Android, you cannot name a property flex or the app will crash" ?

@cpojer

@cpojer
Copy link
Contributor Author

cpojer commented Apr 26, 2019

Hey @matheusmonte, give it a try! Unfortunately I don't have more context myself but maybe @alburdette619 (who mentioned this initially) can provide a repro?

@zhongwuzw
Copy link
Contributor

https://twitter.com/estevao_lucas/status/1117572702083190785?s=215

@cpojer I'll enhance the compatible for this error.

@bryankang
Copy link

@sasurau4 @cpojer I don't know how I feel about automatically converting width: "100" to width: "100%" automatically. I think that it adds a bit of unwanted magic and potentially lead to hard to find bugs.

I personally make sure to always wrap the percentage values in quotes, as do most people, so I think this feature is unnecessary.

@cpojer
Copy link
Contributor Author

cpojer commented Apr 29, 2019

@zhongwuzw thank you!

@bryankang I agree with you. We shouldn't automatically convert, but we should make sure that the app doesn't crash and instead make sure we show a redbox error or something. The initial report said that the app crashes. If it already shows an error, we can make sure that it is more useful and if it already shows a proper redbox error we don't need to make a change.

facebook-github-bot pushed a commit that referenced this issue Apr 29, 2019
Summary:
Fixes https://twitter.com/estevao_lucas/status/1117572702083190785?s=215 in #24626 . Now we try to convert any id to `NSString`, not throw error.

cc. cpojer .

[iOS] [Fixed] - Add convert compatible of NSString for bridge message data
Pull Request resolved: #24630

Differential Revision: D15120205

Pulled By: cpojer

fbshipit-source-id: 4849a8e941410b292f971608a9cdb38c11502445
@estevaolucas
Copy link

@bryankang I agree with you. We shouldn't automatically convert, but we should make sure that the app doesn't crash and instead make sure we show a redbox error or something. The initial report said that the app crashes. If it already shows an error, we can make sure that it is more useful and if it already shows a proper redbox error we don't need to make a change.

@cpojer @bryankang I just made a PR for it, #24659. The propose is just to ignore any unexpected value. No error will show up. (AFAIK any error will cause the app to crash in release mode). Let me know what you guys think.

@sasurau4 sorry, I noticed just after I submitted this PR that you volunteered to fix it. My bad. :-(

@sasurau4
Copy link
Contributor

@elucaswork Never mind! 👍

@bryankang @cpojer I agree with you about automatically convert may cause something confusing.
According to the #24659, the way to fix this issue is invoke yellow box warning and ignore the invalid value like 100. I think it's better solution rather than my proposal of auto conversion.

@cpojer
Copy link
Contributor Author

cpojer commented Apr 30, 2019

I just added a new item by @jlongster which is about fixing an annoying crash, see https://twitter.com/jlongster/status/1123225841075392514 Feel free to sign up for it and propose a PR with a fix!

@matheusmonte
Copy link

Can I try "Unhelpful error message with ScrollViews. See " @cpojer

@cpojer
Copy link
Contributor Author

cpojer commented Apr 30, 2019 via email

grabbou pushed a commit that referenced this issue May 6, 2019
Summary:
Fixes https://twitter.com/estevao_lucas/status/1117572702083190785?s=215 in #24626 . Now we try to convert any id to `NSString`, not throw error.

cc. cpojer .

[iOS] [Fixed] - Add convert compatible of NSString for bridge message data
Pull Request resolved: #24630

Differential Revision: D15120205

Pulled By: cpojer

fbshipit-source-id: 4849a8e941410b292f971608a9cdb38c11502445
@stale
Copy link

stale bot commented Aug 4, 2019

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Aug 4, 2019
@stale
Copy link

stale bot commented Aug 11, 2019

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to create a new issue with up-to-date information.

@stale stale bot closed this as completed Aug 11, 2019
@facebook facebook locked as resolved and limited conversation to collaborators Aug 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug p: Facebook Partner: Facebook Platform: Android Android applications. Platform: iOS iOS applications. Ran Commands One of our bots successfully processed a command. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

No branches or pull requests

9 participants