cryogen-project / cryogen-core

Cryogen's core
Eclipse Public License 1.0
69 stars 62 forks source link

Added theme config ability #97

Closed KingMob closed 7 years ago

KingMob commented 7 years ago

Added ability for themes to have their own config.edn and specify additional resource and Sass directories.

NB: may want to consider how the merging of theme config works in the long run. It uses merge-with into to combine collections, but this will break if someone adds a theme config option that's a scalar. When overrides uses deep-merge, any key that's not a map is completely replaced by the override; we want to merge things like :sass-src and :resources though, not replace them.

Let me know if you want this a bit more future-proof.

lacarmen commented 7 years ago

It uses merge-with into to combine collections, but this will break if someone adds a theme config option that's a scalar.

Maybe take a look at the schemas and see if anything can be made stricter/more robust?

When overrides uses deep-merge, any key that's not a map is completely replaced by the override; we want to merge things like :sass-src and :resources though, not replace them.

Perhaps deep-merge should be changed to combine collections then.

Let me know what you think @KingMob

KingMob commented 7 years ago

Maybe take a look at the schemas and see if anything can be made stricter/more robust?

@lacarmen, I'm not sure I follow what you're thinking on the schemas. Restricting theme config options to be a limited subset of the main config options? (Which seems like a good idea, now that I think about it.)

Perhaps deep-merge should be changed to combine collections then.

That's probably a good idea, the only catch is it could be a breaking change for anyone using deep-merge for overrides at the moment. If they rely on the behavior of "if not map, last one wins" to replace one options with another...

Looking at #92, @SVMBrown's overrides mention scalars only, which would still exhibit "last one wins" behavior regardless, so maybe it wouldn't be an issue. Perhaps they can chime in?

lacarmen commented 7 years ago

Restricting theme config options to be a limited subset of the main config options? (Which seems like a good idea, now that I think about it.)

That could be a good idea, but I'm not sure if that will get complicated with different themes having different restrictions.

I can't remember what I meant by the schemas now. Perhaps it wasn't a good idea afterall 😛 .

@SVMBrown is going to be away for 3 weeks so I don't expect we'll hear from him for a while.

KingMob commented 7 years ago

That could be a good idea, but I'm not sure if that will get complicated with different themes having different restrictions.

Oh uh, I just meant that themes (all themes) would be restricted to a subset of the main config. Not different schemas for different themes, if that's what you meant.

How about I just alter deep-merge with the following behavior:

  1. Add an override param
  2. With the override flag set, maps will be recursively merged, but for scalars and vectors, the last one passed in replaces the original.
  3. Without the override flag, maps will be recursively merged, vectors will be conjoined, and for scalars, the last one passed in replaces the original.

@SVMBrown may have to update his deep-merge call site, but it'll at least address both use cases.

lacarmen commented 7 years ago

That sounds like the best choice for now. :+1:

KingMob commented 7 years ago

OK, give it a whirl now.

lacarmen commented 7 years ago

I tested this with the new theme and everything looks great! Thanks for all your work, Matthew 😄