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

Improve color resource handling in Common.java #913

Merged

Conversation

andvalsol
Copy link
Contributor

The changes handle the fetching of color resources more accurately in our Common.java file. The old method was simplified to one line, but this didn't consider raw color values. Now, the code correctly handles both cases: when the type value is resolved to a color resource ID, and when it's resolved to a raw color value.

Summary

Handling Default Values
If the textColorPrimary attribute is not explicitly defined in your theme, the typedValue.resourceId might still be 0, and typedValue.data might contain a raw color value instead of a resource ID. This could lead to unexpected behavior if you're expecting a resource ID.
Explicit Check: We now explicitly check if typedValue.resourceId is non-zero before assuming it's a resource ID.
Raw Color Handling: If typedValue.resourceId is 0, we directly use typedValue.data as the color value since it likely represents a raw color.
This modification ensures that your code gracefully handles cases where the resolved attribute value is either a color resource ID or a raw color value.

Test Plan

What's required for testing (prerequisites)?

N/A

What are the steps to reproduce (after prerequisites)?

If there's no default default text color in the App's theme the app that implements this library will crash.

Compatibility

OS Implemented
iOS ✅ ❌
Android ✅ ❌

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)
  • I have added automated tests, either in JS or e2e tests, as applicable

The changes handle the fetching of color resources more accurately in our Common.java file. The old method was simplified to one line, but this didn't consider raw color values. Now, the code correctly handles both cases: when the type value is resolved to a color resource ID, and when it's resolved to a raw color value.
Copy link
Member

@vonovak vonovak left a comment

Choose a reason for hiding this comment

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

can you please
Hello, and thank you for the PR!

Can you please fix the indentation?

Thank you 👍

@andvalsol
Copy link
Contributor Author

@vonovak Hello, I tried to see if there were any identation errors but according to my Android Studio the formatting is correct, could you please tell which identation format are you using?

@vonovak
Copy link
Member

vonovak commented Jul 9, 2024

@andvalsol I'm afraid that different parts of the code base are formatted differently so there isn't a unified formatting, but if you look at the diff tab here in github you'll see it. Just follow the existing indentation and we're fine :)

TY!

Screenshot 2024-07-09 at 9 58 01

@vonovak vonovak merged commit 8527354 into react-native-datetimepicker:master Jul 9, 2024
5 checks passed
vonovak pushed a commit that referenced this pull request Jul 9, 2024
# [8.2.0](v8.1.1...v8.2.0) (2024-07-09)

### Bug Fixes

* **android:** improve color resource handling in Common.java ([#913](#913)) ([8527354](8527354))

### Features

* create expo plugin to customize native style ([#801](#801)) ([66235db](66235db))
@vonovak
Copy link
Member

vonovak commented Jul 9, 2024

🎉 This issue has been resolved in version 8.2.0 🎉

If this package helps you, consider sponsoring us! 🚀

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

Successfully merging this pull request may close these issues.

2 participants