FEATURE: Theme-owned color palettes #32795
Conversation
768923f to
53295b1
Compare
app/assets/javascripts/admin/addon/controllers/admin-customize-themes-show.js
Outdated
Show resolved
Hide resolved
app/assets/javascripts/admin/addon/controllers/admin-customize-themes-show.js
Outdated
Show resolved
Hide resolved
app/assets/javascripts/admin/addon/templates/customize-themes-show.gjs
Outdated
Show resolved
Hide resolved
app/assets/javascripts/admin/addon/controllers/admin-customize-themes-show.js
Outdated
Show resolved
Hide resolved
f903ffa to
3169855
Compare
839286d to
069de0f
Compare
app/assets/javascripts/admin/addon/controllers/admin-customize-themes-show-colors.js
Outdated
Show resolved
Hide resolved
app/assets/javascripts/admin/addon/components/changes-banner.gjs
Outdated
Show resolved
Hide resolved
app/assets/javascripts/admin/addon/components/changes-banner.gjs
Outdated
Show resolved
Hide resolved
508ee6c to
6867ede
Compare
d3ff343 to
01f7913
Compare
| this.toasts.error({ | ||
| data: { | ||
| message: i18n( | ||
| "admin.config_areas.color_palettes.invalid_color_length" | ||
| ), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Personally, I feel like using a toast here is not the right UI pattern. Normally, input validation errors are shown right below the input so that it is clear which input the error is shown for.
There was a problem hiding this comment.
Just to make it clear, I don't think this should be a blocking comment but something worth thinking about.
cc @ellaestigoy
| if (event.keyCode === 13) { | ||
| event.preventDefault(); | ||
|
|
||
| if (currentValue.length !== 6 && currentValue.length !== 3) { |
There was a problem hiding this comment.
Instead of having to manually validate this, I think we can just rely on the minLength and maxLength attributes on the input to restrict the length?
There was a problem hiding this comment.
Also, it has limitation that toast will show only when admin hit enter (13 keyCode). But I can click outside and will never know that something is wrong.
Demo
Screen.Recording.2025-06-03.at.4.17.31.pm.mov
When I am out of focus and value is invalid, should we display error as well?
| @action | ||
| onTextPaste(event) { | ||
| const content = (event.clipboardData || window.clipboardData).getData( | ||
| "text" | ||
| ); | ||
|
|
||
| if (!this.isValidHex(content)) { | ||
| event.preventDefault(); | ||
| this.toasts.error({ | ||
| data: { | ||
| message: i18n( | ||
| "admin.config_areas.color_palettes.invalid_color_length" | ||
| ), | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Having to handle all these events manually feels odd to me. In my mind, the pattern should be that each input tracks its own "value" and reacts accordingly when the value of its input changes.
| data: JSON.stringify({ colors }), | ||
| contentType: "application/json", | ||
| }); | ||
| this.owned_color_palette = paletteData; |
There was a problem hiding this comment.
I'm curious why we need to set this attribute as it doesn't seem to be used anywhere.
| import { service } from "@ember/service"; | ||
| import { i18n } from "discourse-i18n"; | ||
|
|
||
| export default class AdminCustomizeThemesShowColorsRoute extends Route { |
There was a problem hiding this comment.
We need to take note to add a page title whenever we add a new route.
| import { scrollTop } from "discourse/lib/scroll-top"; | ||
| import { i18n } from "discourse-i18n"; | ||
|
|
||
| export default class AdminCustomizeThemesShowIndexRoute extends Route { |
There was a problem hiding this comment.
Same here, we have a missing page title.
| import Service from "@ember/service"; | ||
| import { TrackedSet } from "@ember-compat/tracked-built-ins"; | ||
|
|
||
| export default class ColorPaletteChangeTracker extends Service { |
There was a problem hiding this comment.
I'm not sure if this qualifies as an Ember service which is an Ember object that lives for the duration of the application. In the case of this usage here, I think defining the tracked properties on the controller is a better fit?
| ThemeColorScheme.insert_all( | ||
| [{ theme_id: self.id, color_scheme_id: copy.id }], | ||
| unique_by: :index_theme_color_schemes_on_theme_id, | ||
| ) |
There was a problem hiding this comment.
Just curious, why do we need to use insert_all here when we can just use create! and let the database handle the uniqueness validation.
| # race condition, a palette has already been associated with this theme | ||
| copy.destroy! | ||
| self.reload.owned_color_palette |
There was a problem hiding this comment.
Can you expand on how this race condition can happen?
| palette.colors.each do |color| | ||
| copy_color = color.dup | ||
| copy_color.color_scheme_id = copy.id | ||
| copy_color.save! | ||
| end |
There was a problem hiding this comment.
This seems like a good opportunity to use insert_all since we are trying to insert multiple rows.
| end | ||
|
|
||
| def include_base_palette? | ||
| object.color_scheme_id.blank? && object.owned_color_palette.blank? |
There was a problem hiding this comment.
I don't seem to see owned_color_palette being eager loaded anywhere so this will result in an N+1 queries problem when this serializer is used to render a list of themes.
| if (event.keyCode === 13) { | ||
| event.preventDefault(); | ||
|
|
||
| if (currentValue.length !== 6 && currentValue.length !== 3) { |
There was a problem hiding this comment.
Also, it has limitation that toast will show only when admin hit enter (13 keyCode). But I can click outside and will never know that something is wrong.
Demo
Screen.Recording.2025-06-03.at.4.17.31.pm.mov
When I am out of focus and value is invalid, should we display error as well?
| this.colorPaletteChangeTracker.dirtyColorsCount > 0 && | ||
| transition.intent.name !== "adminCustomizeThemes.show.index" | ||
| ) { | ||
| transition.abort(); |
There was a problem hiding this comment.
I noticed a bug when I played with it. When I change color and try to leave the page, I see a warning. However, if I frist click settings, warning is not showing. Recorded that
Screen.Recording.2025-06-03.at.4.33.58.pm.mov
|
Wow, that is huge amount of work! I played with it and it feels much better than the previous version 🎊 I found and recorded 2 bugs, but because it is hidden behind feature flag, I am ok to merge it like that and improve in next iterations. However, I think that Tgx's comments are valid and important. Finally, I feel that we should not remove feature flag until we solve "revert to original color". But this is problem for another day and let's first close this monster :) |
This PR replaces the color palette dropdown on the theme page with a color palette editor that enables editing the theme's color palette directly from the theme page. With this change, a theme's color palette is strongly tied to its theme and can't be linked to other themes and it can't be selected by users without using the theme as well.
All of the changes are behind a feature flag. To enable it, turn on the
use_overhauled_theme_color_palettesetting.Screenshot: