Give the current theme to widgets and the integration manager #1669
Conversation
This is a namespaced variable because some clients may not be able to support themes, or may have varying definitions of what "light" means. Widgets are recommended to opt for per-client checks, or accept that some clients may differ. Signed-off-by: Travis Ralston <[email protected]>
For integration managers which would like to theme themselves to match Riot. Signed-off-by: Travis Ralston <[email protected]>
|
A possible future improvement might be to send the actual theme, rather than just the name of it. See e.g element-hq/element-web#5844 for discussion in how to store/ serialize themes. |
|
LGTM, assigning to Rick who's best suited to saying whether this will have any adverse effects with Scalar |
|
@turt2live - Thanks for this. Another really useful feature that I have been meaning to get to for a while. I like @pafcu's suggestion of serializing, or passing the actual theme in some way (but that's probably going to need quite a bit more thought / implementation, further down the line). I get your reasoning behind namespacing the theme parameter in this way, but (having discussed it at quite some length), we feel that it is probably better to namespace the themes themselves in some way, rather than having to handle parameters for every possible client (we hope / envisage that there will be lots in future). Can you please change the theme parameter to "theme" for the time being? For now the integration managers should be able to handle the minimal number of themes in the wild. Ultimately, as themes proliferate, we should standardise on some theme name-spacing e.g. "riot.dark", "riot.light" which will allow the integration managers to render the actual theme "riot.dark", if available, but if not, fall back on to it's general "dark theme", which is more likely to be suitable and doesn't require knowledge of every possible client? |
| @@ -131,6 +132,9 @@ module.exports = React.createClass({ | |||
| '$matrix_room_id': this.props.room.roomId, | |||
| '$matrix_display_name': user ? user.displayName : this.props.userId, | |||
| '$matrix_avatar_url': user ? MatrixClientPeg.get().mxcUrlToHttp(user.avatarUrl) : '', | |||
|
|
|||
| // Namespaced for Riot | |||
| '$riot_theme': SettingsStore.getValue("theme"), | |||
There was a problem hiding this comment.
Can we please change '$riot_theme' to '$theme' -- See main thread comment.
There was a problem hiding this comment.
The solution of namespacing themes makes a lot more sense. Should the variable instead be $matrix_theme or just change it to $theme?
There was a problem hiding this comment.
After thinking about it for a bit, I decided not to because it's not related to matrix - it's related to the client only.
@rxl881 updated.
Signed-off-by: Travis Ralston <[email protected]>
For cases where the widgets and/or integration manager wish to look similar to Riot. This is kinda a step forward to the occasional complaint of Scalar not being dark-theme capable (I can't find the issue for this). Someone else would actually have to do the dark theme for Scalar, as I obviously don't have access.
The reason for namespacing the theme parameter in the widget URL is because other clients may not have the concept of themes, or may not agree on what their identifiers should be. For instance, another client may decide that their "light theme" is actually called "our_awesome_light_theme". Clients are recommended to document and support similar parameters, and widget writers are recommended to have per-client parameters in their widgets to check for the theme.