FEATURE: User selectable color schemes - #10544
Conversation
eviltrout
left a comment
eviltrout
left a comment
There was a problem hiding this comment.
I'm mostly concerned about the two set operations that do nothing. Otherwise this is in pretty good shape.
| return I18n.t("user.color_schemes.default_description"); | ||
| }, | ||
|
|
||
| selectedColorSchemeId: computed({ |
There was a problem hiding this comment.
Why is this set up to do a no-op here?
There was a problem hiding this comment.
Yeah, no good reason. My goal was to have selectedColorSchemeId take the value from the session as its default without updating the source, but it wasn't done right.
I have refactored this now, with help from @jjaffeux, it's much cleaner, the values are set in the controller's init(). Another nicety: the session values don't need to be transiting via the preferences-interface route.
| textSize: user.get("currentTextSize"), | ||
| themeId: currentThemeId(), | ||
| userColorSchemeId: | ||
| parseInt(Session.currentProp("userColorSchemeId"), 10) || null, |
There was a problem hiding this comment.
You should be able to say this.session.userColorSchemeId without importing it.
| return listColorSchemes(this.site); | ||
| }, | ||
|
|
||
| @discourseComputed("userSelectableColorSchemes") |
There was a problem hiding this comment.
pretty sure you can one line it:
showColorSchemeSelector: reads("userSelectableColorSchemes.length")
| return value; | ||
| }, | ||
| get() { | ||
| return this.get("userColorSchemeId"); |
There was a problem hiding this comment.
do we need the get here ?
| return colorSchemes && colorSchemes.length; | ||
| }, | ||
|
|
||
| @discourseComputed |
There was a problem hiding this comment.
selectedColorSchemeNoneLabel: i18n("user.color_schemes.default_description"),
| return value; | ||
| }, | ||
| get() { | ||
| return this.get("userDarkSchemeId"); |
There was a problem hiding this comment.
think we can get rid of the get here
| }); | ||
|
|
||
| if (colorSchemeId < 0) { | ||
| const defaultTheme = this.userSelectableThemes.find( |
There was a problem hiding this comment.
findBy("id", this.themeId)
| "model.user_option.dark_scheme_id", | ||
| this.enableDarkMode ? null : -1 | ||
| ); | ||
| const makeColorSchemeDefault = this.makeColorSchemeDefault; |
There was a problem hiding this comment.
It's a nitpick and up to you, but I tend to avoid creating a variable in this case, I think the fact that you have to grep for two cases instead of one now largely surpasses the 5 chars
3 participants
This lets administrators mark color schemes as
user_selectable, which then are displayed as options in the user's Preferences > Interface screen. Users can pick a regular color scheme and (if dark color schemes are available) a dark mode color scheme.Behind the scenes this PR:
user_selectablecolumn to thecolor_schemestablecolor_scheme_idcolumn touser_options(adark_scheme_idcolumn was already added a few weeks ago)user_optionstable (similar to theme/text size choices)color-scheme-picker.jslibrary that can be used to build similar widgets in plugins/theme components (for example, to toggle color scheme choices in the hamburger menu)color-scheme-stylesheetroute that facilitates previewing users' color scheme choices on-the-fly