feat: Open edX app theming capability improvements (Option 2) #182
Conversation
saeedbashir
requested review from
volodymyr-chekyrta,
rnr and
eyatsenkoperpetio
|
@volodymyr-chekyrta @rnr @eyatsenkoperpetio I've created this PR with a different approach. Hopefully, this approach will work because we are already following the framework approach, and I've created another framework, |
I'm ok with both approach where we break down Assets on static and configurable since we need to change it with whitelabel script (but even without dividing whitelabel script can change assets just needs more configs perhaps). Using 'framework' way looks more structural for me. I'm waiting for @volodymyr-chekyrta inputs here. |
|
@touchapp would love to have your thoughts on this approach as well. |
|
@saeedbashir makes sense to me 🙌 One concern. We probably need to link frameworks to each other in the same way as we do with other modules. |
What benefit will we get after linking the theme framework to any other framework, like to Core framework? |
There are no benefits, that just shouldn't work that way. So, the question arises - how does this work, especially since the prior version of XCode would have thrown an error in this situation 🤔 |
I'm not sure, but it might be working because it's linked with the OpenedX target along with other frameworks. Anyhow, I've linked the Theme framework with Core framework. 
|
|
@volodymyr-chekyrta Can you please look into this PR, there is a big change-set because of structual change. If we don't merge it early, most probably every merge will result in a conflict. |
I'm currently waiting for reviews from @rnr and @eyatsenkoperpetio, but I'll take a closer look today to speed up this process 🤝 |
|
@saeedbashir, the project is building to the device, but the Archive build is not working. Could you please check it? 
|
OpenEdX/Environment.swift
Outdated
| @@ -0,0 +1,89 @@ | |||
| // | |||
| // Environment.swift | |||
There was a problem hiding this comment.
@saeedbashir this file is still existing in OpenEdX folder. I believe it should be deleted. Thank you
It is fixed; the debug statement was missing for Preview. |
Thank you! I'll check a little later |
| fonts = loadANdParseFonts() | ||
| } | ||
|
|
||
| private func loadANdParseFonts() -> [String: String] { |
There was a problem hiding this comment.
Misspelling
loadANdParseFonts -> loadAndParseFonts
In this PR, I've made the changes that will support app theming. Below are the changes that I made:
Multiple colors and images
I've broken down the xcassets folder of Core Framework and move the configureables (most likely those will change for theming of openedX app) and moved them into a new framework
Theme. From now on, Core framework will contain all the images, icons, colors, etc, that won't change with theming of the app, like button icons, nav bar icons, and bottom bar icons. Likewise,Theme Assetswill contain all the changeable 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.
Note: It's the second PR for making the theming easier. Another PR with different approach is already created and available here: #173