FEATURE: Theme-owned color palettes #32795
Conversation
0026468 to
768923f
Compare
768923f to
53295b1
Compare
| } | ||
|
|
||
| @action | ||
| onDarkColorChange(name, value) { |
There was a problem hiding this comment.
I'm wondering if we can just pass the color object to this callback instead of having to loop through again in the controller to retrieve the object.
| @@ -476,4 +488,26 @@ export default class AdminCustomizeThemesShowController extends Controller { | |||
| editColorScheme() { | |||
| this.router.transitionTo("adminCustomize.colors.show", this.colorSchemeId); | |||
| } | |||
|
|
|||
| @action | |||
| onLightColorChange(name, value) { | |||
There was a problem hiding this comment.
| <DButton | ||
| class="admin-theme__save-palette-changes btn-primary" | ||
| @icon="check" | ||
| @action={{@controller.saveColorChanges}} | ||
| /> |
There was a problem hiding this comment.
I think it will be useful to add a disable state for this button since it involves an AJAX call.
| @discourseComputed( | ||
| "model.colorPalette.colors.@each.{hex,originalHex,dark_hex,originalDarkHex}" | ||
| ) | ||
| hasChangedColors() { | ||
| return this.model.colorPalette.colors.some((color) => { | ||
| return ( | ||
| color.hex !== color.originalHex || | ||
| color.dark_hex !== color.originalDarkHex | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Instead of having to inspect each object for changes, perhaps we can pass a tracked property to the ColorPaletteEditor component for it to update this tracked property when changes are made?
| @@ -76,6 +60,10 @@ export default class ColorScheme extends EmberObject { | |||
| init() { | |||
| super.init(...arguments); | |||
|
|
|||
| const colors = A(this.colors ?? []); | |||
| this.colors = colors.map((c) => { | |||
There was a problem hiding this comment.
All these looping in the PR is also making me wonder if we selected the wrong data structure here. Perhaps we should be using a object here instead of an array so that we can easily look up a color object by its name?
| it "creates a theme-owned color palette if one doesn't exist" do | ||
| expect(theme.owned_color_palette).to be_nil | ||
|
|
||
| max_id = ColorScheme.maximum(:id) || 0 |
There was a problem hiding this comment.
| max_id = ColorScheme.maximum(:id) || 0 |
This line is not used.
| it "updates an existing theme-owned color palette" do | ||
| palette = theme.find_or_create_owned_color_palette | ||
| color = palette.colors.find_by(name: "primary") | ||
|
|
||
| put "/admin/themes/#{theme.id}/change-colors.json", | ||
| params: { | ||
| colors: [{ name: "primary", hex: "aabbcc", dark_hex: "ccddee" }], | ||
| } | ||
|
|
||
| expect(response.status).to eq(200) | ||
| color.reload | ||
| expect(color.hex).to eq("aabbcc") | ||
| expect(color.dark_hex).to eq("ccddee") | ||
| end | ||
|
|
||
| it "only updates specified colors" do | ||
| palette = theme.find_or_create_owned_color_palette | ||
| primary_color = palette.colors.find_by(name: "primary") | ||
| secondary_color = palette.colors.find_by(name: "secondary") | ||
|
|
||
| original_secondary_hex = secondary_color.hex | ||
|
|
||
| put "/admin/themes/#{theme.id}/change-colors.json", | ||
| params: { | ||
| colors: [{ name: "primary", hex: "112233", dark_hex: "445566" }], | ||
| } | ||
|
|
||
| expect(response.status).to eq(200) | ||
| primary_color.reload | ||
| secondary_color.reload | ||
|
|
||
| expect(primary_color.hex).to eq("112233") | ||
| expect(secondary_color.hex).to eq(original_secondary_hex) | ||
| end |
There was a problem hiding this comment.
These two tests can be combined into one IMO.
| it "shows color palette editor when feature is enabled" do | ||
| theme_page.visit(theme.id) | ||
|
|
||
| expect(theme_page).to have_color_palette_editor | ||
| expect(theme_page).to have_no_color_scheme_selector | ||
| end | ||
|
|
||
| it "doesn't show color palette editor when feature is disabled" do | ||
| SiteSetting.use_overhauled_theme_color_palette = false | ||
| theme_page.visit(theme.id) | ||
|
|
||
| expect(theme_page).to have_no_color_palette_editor | ||
| expect(theme_page).to have_color_scheme_selector | ||
| end |
There was a problem hiding this comment.
Personally, I don't think these two cases are worth the overhead of system tests. The risk of a regression here is also low IMO.
| expect(theme_page).to have_color_scheme_selector | ||
| end | ||
|
|
||
| it "allows editing colors without affecting other themes" do |
There was a problem hiding this comment.
| it "allows editing colors without affecting other themes" do | |
| it "allows editing colors" do |
I don't think we need to verify affecting other themes in system tests. That is already covered by the controller test.
| updated_color = theme_page.color_palette_editor.get_color_value("primary") | ||
| expect(updated_color).to eq("#ff000e") |
There was a problem hiding this comment.
This is not relying on capybara matches so has a good chance to be flaky.
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: