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: add support for dark mode images and semantic colors #11097

Merged
merged 15 commits into from Sep 3, 2019

Conversation

ewanharris
Copy link
Collaborator

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

This introduces support for iOS 13's dark mode. There's two parts to the PR

  • b3ce21f - Adds support for adding a dark mode variant of an image by adding the -dark suffix to a an image name (but before the retina @2x/@3x).
  • f8bd718 - Adds support for the iOS semantic colors, but allows reusability cross platform by loading the semantic.colors.json file on Android/Windows and allowing developers to set a preferred color type via Ti.UI.semanticColorType

https://github.com/ewanharris/darkmode-example can be used as a test/example

@ewanharris ewanharris requested review from cb1kenobi and a team July 31, 2019 14:49
@build build added this to the 8.2.0 milestone Jul 31, 2019
@build build requested a review from a team July 31, 2019 14:51
@build
Copy link
Contributor

build commented Jul 31, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 4380 tests are passing.
(There are 472 tests skipped)

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.

Generated by 🚫 dangerJS against a618323

if ([TiUtils isIOSVersionOrGreater:@"11.0"]) {
return [[TiColor alloc] initWithColor:[UIColor colorNamed:color] name:nil];
} else {
return [[TiColor alloc] initWithColor:UIColor.blackColor name:@"black"];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While this is undocumented, it's still callable by a user. So I kept the fallback to black like in the darkmode plugin/module by Hans.

} else {
if (!colorset) {
try {
colorset = require('/semantic.colors.json'); // eslint-disable-line import/no-absolute-path
Copy link
Contributor

Choose a reason for hiding this comment

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

If we load the JSON file like we do in the link below, then we won't run into a require() error when running via Android Studio, Xcode, or Visual Studio. This way the native dev team here can debug via our IDEs without issue.
https://github.com/appcelerator/titanium_mobile/blob/master/common/Resources/ti.internal/bootstrap.loader.js#L31

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

iOS works fine after 08a592f, I'm also able to build from Android Studio just fine after updating the ti.main.js file in TitaniumTest to one from this PR. I believe as long as rollup is configured correctly it shouldn't be an issue

@vijaysingh-axway
Copy link
Contributor

For color it is working perfect in iOS. I'll check for image

@vijaysingh-axway
Copy link
Contributor

@ewanharris While testing for images -

  1. In iOS 13, if I run app in light mode and go to setting (without killing app) and change to dark mode image is not changing. If I kill the app and run again, it get changed. I tried to add image view and for image view it is changing the image in dark mode and light mode. Probably some issue in SDK. I can check on this. I'll test for images in other component as well.
  2. In iOS < 13, image is not changing because iOS < 13 does not support dark mode. Can we make a mapping similar to color.json for images as well? So that we can have it for iOS < 13 as well.
  3. I guess, we need a wiki page for explaining this and an example app.

@jquick-axway
Copy link
Contributor

I think what @ewanharris is shooting for is that we should also have an "event" that gets fired when the OS' dark/light theme has changed. The theme event listener can then call the fetchSemanticColor() method and re-apply colors to all the views. Right?

@hansemannn
Copy link
Collaborator

@jquick-axway Nope, images and colors are automatically changed by iOS if the appearance is changed - at least it does so natively. But I've added a traitcollectionchange event in some other PR is triggered if the appearance changes, so other changes to the app can be made as well (e.g. analytics)

@jquick-axway
Copy link
Contributor

Nope, images and colors are automatically changed by iOS if the appearance is changed

I get that, but this PR's fetchSemanticColor() method is documented to return a color string type in its *.yml file... which would suggest to anyone only reading our API docs that you would have to re-fetch the color string if the theme changes and re-apply it to all views. (Back when I used to do Windows desktop development, this is exactly what you had to do too.)

I can see in the *.m file that it returns a TiColor object instead of a string. So, what's the real truth here?

@ewanharris
Copy link
Collaborator Author

So on iOS 13+, it returns a TiColor object (how do we even document that?), which will handle the dark mode setting change for a user. In every other situation, the user will get the string from the JSON object they gave us.

I totally overlooked that the UI wont refresh on anything other than iOS 13, but I'm not sure how we'd even achieve that?

@vijaysingh-axway For your point 2, are you thinking about a function that returns the correct image for a given dark mode setting? I think in theory that should just be prefixing -dark onto the image based on the naming conventions we have.

@jquick-axway
Copy link
Contributor

So on iOS 13+, it returns a TiColor object (how do we even document that?), which will handle the dark mode setting change for a user.

Okay. That's fine. We just need to document this, because this is not the behavior I would have expected. Perhaps document that it returns an undocumented color object type?

I say this because Android and Windows do not have an equivalent. On those platforms, you set literal color values. (On Android, we could theoretically create our own custom Drawable class which dynamically changes color, but this is not how it's usually done on that platform... and we might run into some limitations with some of Google's view classes.)

@hansemannn
Copy link
Collaborator

Android Q supports dark mode as well (btw, it went to final beta today). Natively, it's managed by themes, on Titanium: Dunno. Maybe by listing to the change event and re-tint internally? I hope there is a better native way on Android.

@vijaysingh-axway
Copy link
Contributor

@ewanharris For color we have made it easy for developers, just add a resource file semantic-color.json and use it. It will work perfect for iOS < 12. Can we make a similar file for image resources, which have attribute for dark and light similar to colors.
In case iOS < 13, return image named for 'dark' from this for dark mode and for light mode return image name for 'light'. For iOS 13+, we do not have to worry.
One more advantage of using this approach is that we do not have to force developers to make image name as '-dark' extension. It will have many to one mapping for light and dark.
e.g Let's say we have an image name image1.jpg and second image as image2.jpg and I want to use same image (darkimage.jpg) in dark mode for both. In current implementation developer have to create a copy of same image and make it image1-dark.jpg and image2-dark.jpg. But if we make mapping developer do not have to do that. Same image will work.
@hansemannn @jquick-axway your thoughts?

@ewanharris For my point 3 in previous comment, I was say a wiki page for explanation of how developer can implement dark mode in there project.

@janvennemann
Copy link
Contributor

@vijaysingh-axway I don't think an additional file for images is necessary. IMHO adhering to a specific naming pattern is easier than having an additional mapping file to maintain which only has the benefit of being able to configure the image names. I mean how likely is it that two different light mode images have the same dark mode image?

@hansemannn
Copy link
Collaborator

Same here, we should go with naming conventions to eave the development process. For us, we have 130+ unique images and no issues with the images using the dark- suffix.

@ewanharris
Copy link
Collaborator Author

@vijaysingh-axway, I agree with the others, I think promoting a convention like file naming is preferable to configuration via a json file, I do agree that it isn't as easy to transfer cross-platform as the json file though.

Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

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

Looks good.

@ewanharris
Copy link
Collaborator Author

@vijaysingh-axway, on the docs I started drafting something this week which I'll aim to finish next week. I'll adapt to the repo a thttps://github.com/ewanharris/darkmode-example into something we can move to the appcelerator-developer-relations org and see about updating KitchenSink too

This adds a cross platfom method for loading semanitc colors, on iOS 11+ we will use the native Ti.UI.iOS.fetchSemanticColor to load the right color, in all other cases we use the Ti.UI.semanticColorType and the provided json file to obtain the correct value

Fixes TIMOB-27126
Due to the way Ti.UI works on Android we cant reliably track the changes, by implementing the property with the get/set syntax we can track the property changes ourselves and reliably return the correct result
@keerthi1032
Copy link
Contributor

FR Passed.SemanticColor is supported.fetches color and displayed based on dark mode/light mode selected on 13 device. images displayed correctly for dark and light mode.
Test Environment:
Name = Mac OS X
Version = 10.14.5
Architecture = 64bit
Node.js
Node.js Version = 10.16.2
npm Version = 6.9.0
Titanium CLI
CLI Version = 5.2.1
Titanium SDK
SDK Version =localSDK 8.2.0.v20190902191630
Device -iPhone XR iOS 13, iPhone X iOS 11(for iOS 11 app runs normal )
Simulator -iPhone 8 iOS 13,

@keerthi1032 keerthi1032 merged commit 1a8ae85 into tidev:master Sep 3, 2019
@ewanharris ewanharris deleted the TIMOB-27126 branch September 5, 2019 09:57
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

7 participants