Remove MainTheme #685
Remove MainTheme #685
Conversation
MainTheme has been removed and should be implemented on the app side if needed (see devbox/components/current-theme.js or devbox/apps/Theme.js for examples). This is because having sub themes in an app is not something people use at the moment, and being able to set the main theme as a prop makes things easier to manage for custom themes. Custom themes are also always extending the base theme corresponding to their appearance, making the process of extending themes a bit easier.
|
Any news on this ? |
There was a problem hiding this comment.
We've been discussing offline how to deal with Main.
For now, let's go with what's available in this PR'; I assume all apps implementing dark mode will have to provide the value from useGuiStyle() to their Main declarations.
In the future, I think the best API for Main is to keep these props as strict "overrides" (so the current API is still useful), but change the defaults based on the context. In the case of an Aragon App, perhaps we can base the layout behaviour based on the UI services provided (or some other mechanism).
| if (typeof theme === 'string' && EMBEDDED_THEMES[theme]) { | ||
| return EMBEDDED_THEMES[theme] | ||
| } | ||
| return theme | ||
|
|
||
| if (theme === EMBEDDED_THEMES[theme._name]) { |
There was a problem hiding this comment.
We may want to check deep equality here against all keys, or comment that we're only interested in this particular shortcut.
There was a problem hiding this comment.
I’m not sure I follow, do you mean checking against another entry in a theme? The idea here is just to check if the requested theme is available in the embedded ones (basically, is it light or dark). The only accepted value would be one that correspond to the _name of the theme.
There was a problem hiding this comment.
I'm just worried you'll use a different object reference for theme, even if the theme is the exact same.
The idea here is just to check if the requested theme is available in the embedded ones (basically, is it light or dark). The only accepted value would be one that correspond to the _name of the theme.
Would if (EMBEDDED_THEMES[theme._name]) work?
There was a problem hiding this comment.
Oh right! Sorry I was mistaking with the previous check on the name 😐
Would
if (EMBEDDED_THEMES[theme._name])work?
Yes! And I’m thinking now that we might want to remove this entirely? If someone passes a theme that has the same name than one of the embedded ones, we probably want to use the one being passed anyway?
| } | ||
| <Main theme={theme}> | ||
| <Header primary="Theme" /> | ||
| <Bar primary={<BackButton />} secondary={<Button label="Share" />} /> |
There was a problem hiding this comment.
Maybe we could remove the share button for now, since it doesn't do anything?
There was a problem hiding this comment.
The idea is more to preview the theme, rather than having the components do something :-)
| @@ -1,140 +1,48 @@ | |||
| import React from 'react' | |||
There was a problem hiding this comment.
Not sure about this new Theme page; maybe we can just consolidate the Color and Theme pages together?
There was a problem hiding this comment.
It’s really to develop, it’s not meant to be public. I’m thinking about maybe evolving it as a theme editor at some point, but for now it’s a q&d preview of a theme.
Yes, totally agree on keeping both ways to set a theme. |
This is the main commit: https://github.com/aragon/aragon-ui/commit/34993b5ad38448da77cc25d07e801ba5b470b341