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

[Breaking change] Android: Ti.UI.Color parity #13273

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

Conversation

drauggres
Copy link
Contributor

@drauggres drauggres commented Feb 17, 2022

  • Ti.UI.fetchSemanticColor returns Ti.UI.Color
  • Support Ti.UI.Color instances for components properties
  • Automatically change color value on dark/light mode switch

This PR includes commit from #13267 and #13265
Fix #13272

Read this before merging.

@build
Copy link
Contributor

build commented Feb 17, 2022

Fails
🚫 Test reports missing for Android Main. This indicates that a build failed or the test app crashed
Warnings
⚠️ There is no linked JIRA ticket in the PR body. Please include the URL of the relevant JIRA ticket. If you need to, you may file a ticket on JIRA
Messages
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 14752 tests are passing.
(There are 934 skipped tests not included in that total)

📖 🎉 Another contribution from our awesome community member, drauggres! Thanks again for helping us make Titanium SDK better. 👍

Generated by 🚫 dangerJS against 7fc7d01

@jquick-axway
Copy link
Contributor

FYI: The reason we didn't add Ti.UI.Color proxy support to Android is because it would be a HUGE breaking-change to existing modules since many of them blindly cast a color property's value to (String), which would cause a casting exception.

For example, your changes would break the following...
https://github.com/appcelerator-modules/ti.map/blob/master/android/src/ti/map/CircleProxy.java#L163
https://github.com/appcelerator-modules/ti.map/blob/master/android/src/ti/map/PolygonProxy.java#L122
https://github.com/appcelerator-modules/ti.admob/blob/master/android/src/ti/admob/AdmobView.java#L116
https://github.com/appcelerator-modules/titanium-web-dialog/blob/master/android/src/ti/webdialog/Utils.java#L68

The backward compatible solution that we implemented embeds both dark/light color values into a single string when calling fetchSemanticColor(). That's the "ti.semantic.color:dark=value;light=value" string format I'm sure you've seen. That's the color string you want to assign to a view proxy's properties so that it will support dark/light theme switching... and this definitely works.

Anyways, that's my 2 cents. This March, you guys can handle this anyway you want. If you want to move forward with this, then my recommendation is to do this in a new Titanium 11.0.0 release and force everyone to rebuild their modules.

@drauggres
Copy link
Contributor Author

Thanks @jquick-axway, I'm glad you are still with us.

@drauggres drauggres changed the title Android: Ti.UI.Color parity [Breaking change] Android: Ti.UI.Color parity Feb 18, 2022
@drauggres drauggres marked this pull request as ready for review February 18, 2022 12:00
@hansemannn
Copy link
Collaborator

Very impressive PR! So thrilled to get this reviewed and merged when TiDev can take over.

@hansemannn
Copy link
Collaborator

@drauggres A few merge conflicts to resolve here, but no time pressure as it's for 11.0.0 (targeted for September) anyways.

- "Ti.UI.fetchSemanticColor" returns "Ti.UI.Color"
- Support "Ti.UI.Color" instances for components properties

fix tidev#13272
@drauggres
Copy link
Contributor Author

A few merge conflicts to resolve here, but no time pressure as it's for 11.0.0 (targeted for September) anyways.

This happened bacause this PR had to include commits from #13267 and #13265 and they were "merged" with rebase, i.e. not really merged (in git terms) but copies of the commits were created.

I rebased my branch, should be fine now.

@hansemannn hansemannn modified the milestones: 10.2.0, 11.0.0 Apr 6, 2022
@hansemannn
Copy link
Collaborator

@m1ga And this one as well

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.

Android: Ti.UI.Color parity
5 participants