Add support for library themes #252
Conversation
|
Added a way to add additional keys to be validated for custom themes. |
|
With these changes we can now validate once during compile. We shouldn't need to validate in the viewport anymore. Is my thinking here correct? |
| @@ -278,11 +278,11 @@ defmodule Scenic.Component.Button do | |||
| ) | |||
| end | |||
|
|
|||
| defp do_special_theme_outline(graph, :dark, border) do | |||
| defp do_special_theme_outline(graph, {:scenic, :dark}, border) do | |||
| Graph.modify(graph, :btn, &update_opts(&1, stroke: {1, border})) | |||
| end | |||
|
|
|||
There was a problem hiding this comment.
I'm not sure about these, It doesn't seem like they will ever match. I left them as is since I was already making such a sweeping change.
| @@ -395,7 +395,7 @@ defmodule Scenic.ViewPort do | |||
| def set_theme(viewport, theme) | |||
|
|
|||
| def set_theme(%ViewPort{pid: pid}, theme) do | |||
| case Theme.validate(theme) do | |||
| case Themes.validate(theme) do | |||
There was a problem hiding this comment.
I think validating in the viewport is no longer necessary if the validate is moved to compile time.
|
Love the Palettte module. lets do a call so you can walk me through the rest. Is a bit much to wrap my head around. |
|
Okay, made the changes you requested. Moved the functions with logic out of the macro, and it will now fallback to Scenic.Themes as a default if no themes config is set. |
| error -> error | ||
| end | ||
| end | ||
| nil -> |
There was a problem hiding this comment.
I'm worried that there are many other projects that already use the standard themes of :primary, :secondary etc that will break as they need to be updated to {:scenic, :primary}. Instead of just nil -> it feels like if the theme_name is just an atom it should try {:scenic, :theme_name} here and see if it works. If that fails too, then go to the error.
There was a problem hiding this comment.
This would still allow the dev to override the meaning of :primary, since that would be found first.
There was a problem hiding this comment.
@boydm Personally deprecations are ok for me having in mind that scenic is below 1.0.0 version … Only after that I would expect change of first version number in order to introduce deprecation. I believe that every developer using scenic have in mind it's current state and it's not using it because this project have unbelievably stable API, but because it have amazing features other projects does not provide. 😃
There was a problem hiding this comment.
Went ahead and added backwards compat for single atom theme names for normalize/1, preset/1, validate/1
There was a problem hiding this comment.
It looks like normalize/1 and preset/1 are exactly the same. Maybe one of these can trigger validation and a raise and the other not?
Probably normalize should not do validation as you've probably gotten the tuple directly from the viewport. Whereas with preset you are probably manually passing a theme and could be error prone.
Description
The pull request adds support for registering library themes, similar to static assets.
Themes are now registered and validated within scenic from
Scenic.Themes.Motivation and Context
Before the theme module was on
Scenic.Primitive.Style.Theme, which felt like it would only be used in the context of primitives/components. However, it was imported everywhere that theme validation was needed, and had all the themes directly on that module. This means Scenic would not be able to validate custom themes that were not a map.Also, there were times when scenic would pass around
:dark,:lightatoms, you had to know this ahead of time and know to validate or get the preset viaScenic.Primitive.Style.Theme. With all themes now registered through a single module, you can now fetch presets and validate any theme fromScenic.Themes.Types of changes
not work as expected)
but make things better)
Todos