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

feat: support semantic/named colors for color properties #11699

Merged
merged 35 commits into from Jul 6, 2020

Conversation

hansemannn
Copy link
Collaborator

@hansemannn hansemannn commented May 10, 2020

JIRA: https://jira.appcelerator.org/browse/TIMOB-27895

Notes:

  1. The color lookup is fully backwards compatible for iOS < 11 as well
  2. Android does not have a version limitation, since named colors are available since Android API level 1

This adds the following:

  • a new Ti.UI.Color proxy on Android
    • this brings some parity with iOS (Android doesn't support passing these to color properties directly like iOS does)
    • right now the only real API here is the toHex() method that returns the color's hex string as ARRGGBB format
  • Ti.UI.Android.getColorResource() which can take a numeric argument (for the resource id for the color) or a string argument (for the short color name).
    • i.e. Ti.UI.Android.getColorResource(Ti.Android.R.color.darker_gray) or Ti.UI.Android.getColorResource('holo_blue_bright')
    • it will return a Ti.UI.Color proxy instance that wraps the underlying android.graphics.Color instance
  • color properties can have named colors passed in to them (in addition to our existing support for hex strings, rgb(), rgba() and some of the standard web color names like 'cyan', 'magenta', etc)
  • Ti.UI.fetchSemanticColor() will fall back to using Ti.UI.Android.getColorResource() under the hood if the name isn't in the semantic.colors.json file but there is a Ti.Android.R.color[name] entry (and will still return a valid string to be passed to a color property, should be a hex string in this case).

@build build added this to the 9.1.0 milestone May 10, 2020
@build build requested a review from a team May 10, 2020 08:32
@build
Copy link
Contributor

build commented May 10, 2020

Fails
🚫 Tests have failed, see below for more information.
Messages
📖

💾 Here's the generated SDK zipfile.

📖 🎉 Another contribution from our awesome community member, hansemannn! Thanks again for helping us make Titanium SDK better. 👍
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 1 tests have failed There are 1 tests failing and 698 skipped out of 7367 total tests.

Tests:

ClassnameNameTimeError
android.emulator.Titanium.Androidactivity callbacks (9)5.012
Error: timeout of 5000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53120)

Generated by 🚫 dangerJS against 85011fe

@hansemannn hansemannn changed the title chore(ios): handle semantic colors in color helper chore(ios/android): handle semantic colors in color helper May 10, 2020
@build build requested a review from a team May 10, 2020 12:58
@build build added the android label May 10, 2020

if (semanticColors == nil) {
NSString *colorsPath = [NSBundle.mainBundle pathForResource:@"semantic.colors" ofType:@"json"];
if (![NSFileManager.defaultManager fileExistsAtPath:colorsPath]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to worry about an encrypted version of the file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are JSON resource files encrypted as well? Looking at a recent production build, it doesn't seem to.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't encrypt JSON files under "Resources". So, we should be okay, but it doesn't hurt to test with a device build just in case.

(We do encrypt our internal bootstrap JSON and app properties JSON though. Our build system is capable of doing this for all JSON files if we wanted to.)

return nil;
}

NSString *currentTraitCollection = TiApp.controller.traitCollection.userInterfaceStyle == UIUserInterfaceStyleDark ? @"dark" : @"light";
Copy link
Contributor

Choose a reason for hiding this comment

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

This new chunk of code feels like it should live in the common JS area, but I don't see how we could do that and have it effectively hijack all the possible places where colors are set (i.e. all the proxies with various color properties).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, that was the reason to do it here. And since the JSON only needs to be loaded once, it doesn't cause an overhead to process the array (which wouldn't be a problem on the JS side as well though).

@sgtcoolguy
Copy link
Contributor

@hansemannn If you keep the SDK's npm packages up to date while developing, it should use a husky hook to auto-format the source on IOS when you commit.

Otherwise, try npm run format:ios to manually force it to fix formatting on IOS source.

@hansemannn
Copy link
Collaborator Author

I updated it, but running npm run format:ios attempted to format 15+ files that I haven't even touched, so I only commited the related ones. Also, for unit tests, I could add it to the mocha-suite, but since I need some resource files for it as well, I would like to consider adding no_tests and perform a pull request to that repo for it.

@sgtcoolguy
Copy link
Contributor

@hansemannn You should be able to plop the tests and Resources files into the tests/Resources dir and it'll get looped in.

@sgtcoolguy
Copy link
Contributor

Note also, the semantic.colors.json file seems to have broken the actual iOS app build?

[2020-05-11T15:28:31.980Z] [ERROR] : �� �** BUILD FAILED **
[2020-05-11T15:28:31.980Z] [ERROR] : �� �The following build commands failed:
[2020-05-11T15:28:31.980Z] [ERROR] : �� �	CpResource /Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11699/titanium-mobile-mocha-suite/scripts/mocha/build/iphone/semantic.colors.json /Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11699/titanium-mobile-mocha-suite/scripts/mocha/build/iphone/build/Products/Debug-iphonesimulator/mocha.app/semantic.colors.json
[2020-05-11T15:28:31.980Z] [ERROR] : �� �(1 failure)
[2020-05-11T15:28:31.980Z]  
[2020-05-11T15:28:31.980Z] Error: Exited unexpectedly with exit code: 1
[2020-05-11T15:28:31.980Z]     at ChildProcess.prc.on.code (/Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11699/titanium-mobile-mocha-suite/scripts/test.js:467:19)
[2020-05-11T15:28:31.980Z]     at ChildProcess.emit (events.js:198:13)
[2020-05-11T15:28:31.980Z]     at maybeClose (internal/child_process.js:982:16)
[2020-05-11T15:28:31.980Z]     at Socket.stream.socket.on (internal/child_process.js:389:11)
[2020-05-11T15:28:31.980Z]     at Socket.emit (events.js:198:13)
[2020-05-11T15:28:31.980Z]     at Pipe._handle.close (net.js:607:12)
script returned exit code 1

@hansemannn
Copy link
Collaborator Author

@sgtcoolguy I will check that one! I thought it might be cool to have a semantic.colors.json in development as well, but it seems like some refs of it are taken over to the app template and cause duplicate resource issues there.

@hansemannn
Copy link
Collaborator Author

I'm afraid that I won't be able to update the sample app and remove the demo files from the project before next week, so we may need to close this or hand it over!

@hansemannn
Copy link
Collaborator Author

@sgtcoolguy I removed the file again and added a (very basic) mocha test: https://github.com/appcelerator/titanium-mobile-mocha-suite/pull/255

@sgtcoolguy
Copy link
Contributor

Relates to #11315


NSString *currentTraitCollection = TiApp.controller.traitCollection.userInterfaceStyle == UIUserInterfaceStyleDark ? @"dark" : @"light";

return [Webcolor webColorNamed:colorMap[currentTraitCollection]];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will cause an infinite loop...

@sgtcoolguy sgtcoolguy self-assigned this May 28, 2020
@sgtcoolguy sgtcoolguy changed the title chore(ios/android): handle semantic colors in color helper feat: support semantic/named colors for color properties May 28, 2020
@sgtcoolguy sgtcoolguy force-pushed the TIMOB-27895 branch 3 times, most recently from 5180ae0 to 226ee8f Compare June 2, 2020 18:05
Alos support ArrayBuffer.isView and BYTES_PER_ELEMENT sniffing pixelmatch does
gets pngjs and browserify zlib working
**/
async function getSDKInstallDir() {
// TODO: Use fork since we're spawning off another node process
const { stdout, _stderr } = await exec(`node "${titanium}" info -o json -t titanium`);
Copy link
Contributor

Choose a reason for hiding this comment

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

  • ⚠️ tests/scripts/test.js line 79 – '_stderr' is assigned a value but never used. (no-unused-vars)

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.

None yet

5 participants