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
| @@ -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?
There was a problem hiding this comment.
I've eliminated all the loops except for this one. I think we'd still need to loop here even if we changed the data structure because this converts the data from POJOs to ColorSchemeColor objects.
| 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.
There was a problem hiding this comment.
I think in this particular case it should be fine given that we're reloading the page just before this line and we're testing that the color input is rendered with this value. If we were testing that the color input is changed in reaction to something, then yeah we'd have a flaky test here.
f903ffa to
3169855
Compare
531c599 to
4e4de6c
Compare
23cd630 to
839286d
Compare
839286d to
069de0f
Compare
| get pendingChangesSaveLabel() { | ||
| return i18n("admin.customize.theme.save_colors"); | ||
| } | ||
|
|
||
| get pendingChangesDiscardLabel() { | ||
| return i18n("admin.customize.theme.discard_colors"); | ||
| } |
There was a problem hiding this comment.
we dont need getters is not going to be recomputed
| @action | ||
| discard() { | ||
| this.args.discard(); | ||
| } |
There was a problem hiding this comment.
can probably just use @discard in the template
| @action | ||
| setupResizeObserver(element) { | ||
| const container = document.getElementById("main-container"); | ||
| this._resizer = () => this.positionBanner(container, element); | ||
|
|
||
| this._resizer(); | ||
|
|
||
| this._resizeObserver = window.addEventListener("resize", this._resizer); | ||
| } | ||
|
|
||
| @action | ||
| teardownResizeObserver() { | ||
| window.removeEventListener("resize", this._resizer); | ||
| } |
There was a problem hiding this comment.
I would just use a modifier so you do all in one function and don't need this _resizer variable
| @@ -174,6 +185,34 @@ class Theme extends RestModel { | |||
| return field ? field.error : ""; | |||
| } | |||
|
|
|||
| async changeColors() { | |||
| const payload = []; | |||
There was a problem hiding this comment.
I suggest to call it colors
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: