From 9f21168d68b6e28d4cba70702363022cd36ac17d Mon Sep 17 00:00:00 2001 From: Ryan Shaw Date: Mon, 6 Jun 2016 17:29:44 -0600 Subject: [PATCH] fix so the previews of ThemeCards show the right default colors fixes: CNVS-29360 test plan: *in root account set a primary color purple * go to "Themes" for sub account * the preview for the "Default Theme" should have purple stuff in it * you set other colors / images and make sure those show up in the theme card preview as well Change-Id: Id07899ddf0f473090cd616268b5781d7af318433 Reviewed-on: https://gerrit.instructure.com/81708 Tested-by: Jenkins Reviewed-by: Simon Williams QA-Review: Benjamin Christian Nelson Product-Review: Ryan Shaw --- app/controllers/brand_configs_controller.rb | 3 +++ app/jsx/theme_editor/CollectionView.jsx | 9 +++----- app/jsx/theme_editor/PropTypes.jsx | 6 ++++-- app/jsx/theme_editor/ThemeCard.jsx | 5 +++-- .../jsx/theme_editor/CollectionViewSpec.jsx | 21 +++++++++---------- 5 files changed, 23 insertions(+), 21 deletions(-) diff --git a/app/controllers/brand_configs_controller.rb b/app/controllers/brand_configs_controller.rb index 08f6632ee13..c2e03a4a15b 100644 --- a/app/controllers/brand_configs_controller.rb +++ b/app/controllers/brand_configs_controller.rb @@ -16,8 +16,11 @@ class BrandConfigsController < ApplicationController css_bundle :brand_config_index js_bundle :brand_configs_index + base_brand_config = @account.parent_account.try(:effective_brand_config) + base_brand_config ||= BrandConfig.k12_config if k12? js_env brandConfigStuff: { + baseBrandableVariables: BrandableCSS.all_brand_variable_values(base_brand_config), brandableVariableDefaults: BrandableCSS.variables_map, accountID: @account.id.to_s, sharedBrandConfigs: visible_shared_brand_configs.as_json(include_root: false, include: 'brand_config'), diff --git a/app/jsx/theme_editor/CollectionView.jsx b/app/jsx/theme_editor/CollectionView.jsx index 28a9797c638..8314488400d 100644 --- a/app/jsx/theme_editor/CollectionView.jsx +++ b/app/jsx/theme_editor/CollectionView.jsx @@ -24,7 +24,8 @@ define([ sharedBrandConfigs: React.PropTypes.arrayOf(customTypes.sharedBrandConfig).isRequired, activeBrandConfig: customTypes.brandConfig.isRequired, accountID: React.PropTypes.string.isRequired, - brandableVariableDefaults: customTypes.brandableVariableDefaults + brandableVariableDefaults: customTypes.brandableVariableDefaults, + baseBrandableVariables: customTypes.variables }, getInitialState () { @@ -43,11 +44,7 @@ define([ if (_default && _default[0] === '$') { return this.brandVariableValue(brandConfig, _default.substring(1)) } - const parent = _.find(this.props.sharedBrandConfigs, sbc => sbc.brand_config.md5 === brandConfig.parent_md5) - if (parent) { - return this.brandVariableValue(parent.brand_config, variableName) - } - return _default + return this.props.baseBrandableVariables[variableName] }, startFromBlankSlate() { diff --git a/app/jsx/theme_editor/PropTypes.jsx b/app/jsx/theme_editor/PropTypes.jsx index a839766e7aa..e9de1742482 100644 --- a/app/jsx/theme_editor/PropTypes.jsx +++ b/app/jsx/theme_editor/PropTypes.jsx @@ -19,6 +19,8 @@ define([ variable_name: React.PropTypes.string.isRequired, } + types.variables = React.PropTypes.objectOf(React.PropTypes.string).isRequired + types.color = React.PropTypes.shape(_.extend({ type: React.PropTypes.oneOf(['color']).isRequired }, baseVarDef)) @@ -38,11 +40,11 @@ define([ types.brandConfig = React.PropTypes.shape({ md5: types.md5, - variables: React.PropTypes.object.isRequired + variables: types.variables }) types.sharedBrandConfig = React.PropTypes.shape({ - account_id: React.PropTypes.number, + account_id: React.PropTypes.string, brand_config: types.brandConfig.isRequired, name: React.PropTypes.string.isRequired }) diff --git a/app/jsx/theme_editor/ThemeCard.jsx b/app/jsx/theme_editor/ThemeCard.jsx index 860189f64c4..116bc9dd40b 100644 --- a/app/jsx/theme_editor/ThemeCard.jsx +++ b/app/jsx/theme_editor/ThemeCard.jsx @@ -12,11 +12,12 @@ define([ propTypes: { name: React.PropTypes.string.isRequired, isActiveBrandConfig: React.PropTypes.bool.isRequired, - isDeleteable: React.PropTypes.bool.isRequired, + isDeletable: React.PropTypes.bool.isRequired, isBeingDeleted: React.PropTypes.bool.isRequired, startDeleting: React.PropTypes.func.isRequired, - cancelDelete: React.PropTypes.func.isRequired, + cancelDeleting: React.PropTypes.func.isRequired, onDelete: React.PropTypes.func.isRequired, + getVariable: React.PropTypes.func.isRequired }, render () { diff --git a/spec/javascripts/jsx/theme_editor/CollectionViewSpec.jsx b/spec/javascripts/jsx/theme_editor/CollectionViewSpec.jsx index f98c2e18339..56d3261903a 100644 --- a/spec/javascripts/jsx/theme_editor/CollectionViewSpec.jsx +++ b/spec/javascripts/jsx/theme_editor/CollectionViewSpec.jsx @@ -39,6 +39,9 @@ define([ type: 'color', ovariable_name: 'Other Foo' } + }, + baseBrandableVariables: { + foo: 'red' } } } @@ -54,7 +57,11 @@ define([ this.spy(c, 'brandVariableValue') const config = {variables: {}} - c.brandVariableValue(config, 'otherFoo') + equal( + c.brandVariableValue(config, 'otherFoo'), + props.baseBrandableVariables.foo, + 'follows defaults that start with $ to actual value' + ) ok( c.brandVariableValue.calledWith(config, 'foo'), 'gets value for variable name' @@ -62,16 +69,8 @@ define([ equal( c.brandVariableValue(config, 'foo'), - props.brandableVariableDefaults.foo.default, - 'gets default value' - ) - - const shared = props.sharedBrandConfigs[0].brand_config - config.parent_md5 = shared.md5 - equal( - c.brandVariableValue(config, 'foo'), - shared.variables.foo, - 'get value from parent' + props.baseBrandableVariables.foo, + 'get value from baseBrandableVariables' ) })