Open edX app theming capability improvements #173
Conversation
saeedbashir
force-pushed
the
saeed/app_theme_configurable
branch
from
f9ecbb7 to
e7db1b6
Compare
|
@rnr @volodymyr-chekyrta Please try to review this PR at your earliest convenience because it's breaking the CoreAssets into two, which most probably will result in conflicts too often. |
| @@ -114,7 +114,7 @@ public class CSSInjector { | |||
| <style> | |||
| a { | |||
| text-decoration: none; | |||
| color: \(CoreAssets.accentColor.color.cgColor.hexString ?? ""); | |||
| color: \(Theme.Colors.accentColor.uiColor().cgColor.hexString ?? ""); | |||
| } | |||
| @font-face { | |||
| font-family: "San Francisco"; | |||
There was a problem hiding this comment.
@saeedbashir font-family should be configurable too I think.
thank you
| @@ -0,0 +1,89 @@ | |||
| // | |||
| // Environment.swift | |||
There was a problem hiding this comment.
@saeedbashir was this merged with latest develop? I believe this class was got rid in Config PR. Thank you
There was a problem hiding this comment.
I'm not able to find this class in my branch. It might be showing off because of merge.
There was a problem hiding this comment.
It's strange because I see you merged 'develop' which has this file deleted. Did you do git merge or rebase? Thank you
There was a problem hiding this comment.
I've rebased it. Sometimes I use the github web interface to resolve conflicts, and I guess that uses merge instead of rebase.
There was a problem hiding this comment.
We are using 'merge' most of the time. A lot of articles say it's preferred way when big team is working. Perhaps it helps to avoid situations like this. And save more 'clear' history who/what/when did code
|
Upon reviewing this, I have some concerns about the naming and implementation. From your team's perspective, It might be useful to break down the current Additionally, SwiftGen generated files look a bit odd Note for me: SwiftGen enum names can be fixed with a customized script that will put all resources just in CoreAssets as previously. @saeedbashir @rnr wdyt? |
I was thinking about naming it ConfigurableAssets, but it was a long name.
It would be nice if you could name them so we can also move them to the Configurable.xcassets
This will help out in replacing the changeable / configurable assets, either manually or via script. Once someone makes a copy of their resources, it will be much easier to maintain and change the resources just before making a build with updated codebase.
This is the default behavior of the SwiftGen. I'm new to SwiftGen so don't have in-depth knowledge of SwiftGen. If the enums can be removed (CoreAssets file created just like before in case of single file) this will make the codebase much cleaner. It would be nice if you could help here in writing / using the custom script to achieve above behavior. |
All resources are changeable/configurable. |
In this PR I've made the changes that will support app theming. Belowe are the changes that I made:
Multiple colors and images
I've broken down the
xcassetsfolder of Core Framework into two: xcassetsAssetsandConfigureable. From now on,Assetswill contain all the images, icons, colors, etc that will not be configureable, like button icons, nav bar icons, bottom bar icons. Likewise, configureable assets will contain all the configureable assets.Fonts
I've added an extra layer between the fonts and the theme class. The font names were hardcoded in the theme class, which made it a bit hard to change the fonts in a new theme. I've added a json file for key: name pair and a parser around it. Now the font change will only require the replacement of fonts JSON and font_file and there won't be any code changes.