From 20c4158dfd4007b099a705d3358e25ab221311f3 Mon Sep 17 00:00:00 2001 From: Clay Diffrient Date: Fri, 23 Feb 2018 14:57:21 -0700 Subject: [PATCH] Remove theme editor refactor feature flag closes CORE-690 closes CORE-691 Test Plan: - Theme Editor works completely - Moving between accordion pieces is accessible - There is focus indication between the Edit and Upload tabs when custom css/js is enabled Change-Id: Ica93743b2828c1847d385c477c2f2b4255831fe2 Reviewed-on: https://gerrit.instructure.com/141784 Reviewed-by: Brent Burgoyne QA-Review: Jeremy Putnam Tested-by: Jenkins QA-Review: Tucker McKnight Product-Review: Clay Diffrient --- app/controllers/brand_configs_controller.rb | 3 +- app/jsx/bundles/theme_editor.js | 3 +- app/jsx/theme_editor/ThemeEditor.js | 135 +--------- app/jsx/theme_editor/ThemeEditorAccordion.js | 116 ++------ app/jsx/theme_editor/ThemeEditorColorRow.js | 13 +- app/jsx/theme_editor/ThemeEditorSidebar.js | 2 - .../__tests__/ThemeEditorAccordion.test.js | 1 - lib/feature.rb | 8 - .../theme_editor/ThemeEditorAccordionSpec.js | 253 ------------------ 9 files changed, 35 insertions(+), 499 deletions(-) delete mode 100644 spec/javascripts/jsx/theme_editor/ThemeEditorAccordionSpec.js diff --git a/app/controllers/brand_configs_controller.rb b/app/controllers/brand_configs_controller.rb index 68a7de77696..45c9a76bc15 100644 --- a/app/controllers/brand_configs_controller.rb +++ b/app/controllers/brand_configs_controller.rb @@ -55,8 +55,7 @@ class BrandConfigsController < ApplicationController hasUnsavedChanges: session.key?(:brand_config_md5), variableSchema: default_schema, allowGlobalIncludes: @account.allow_global_includes?, - account_id: @account.id, - REFACTOR_ENABLED: @account.feature_enabled?(:theme_editor_refactor) + account_id: @account.id render html: '', layout: 'layouts/bare' end diff --git a/app/jsx/bundles/theme_editor.js b/app/jsx/bundles/theme_editor.js index d4adbfd4b9c..d8e3e0145a2 100644 --- a/app/jsx/bundles/theme_editor.js +++ b/app/jsx/bundles/theme_editor.js @@ -34,8 +34,7 @@ ReactDOM.render( sharedBrandConfigs: window.ENV.sharedBrandConfigs, allowGlobalIncludes: window.ENV.allowGlobalIncludes, accountID: window.ENV.account_id, - useHighContrast: window.ENV.use_high_contrast, - refactorEnabled: window.ENV.REFACTOR_ENABLED + useHighContrast: window.ENV.use_high_contrast }} />, document.body diff --git a/app/jsx/theme_editor/ThemeEditor.js b/app/jsx/theme_editor/ThemeEditor.js index ecbc8356907..0e290739bab 100644 --- a/app/jsx/theme_editor/ThemeEditor.js +++ b/app/jsx/theme_editor/ThemeEditor.js @@ -76,12 +76,7 @@ export default class ThemeEditor extends React.Component { variableSchema: customTypes.variableSchema, allowGlobalIncludes: PropTypes.bool, accountID: PropTypes.string, - useHighContrast: PropTypes.bool, - refactorEnabled: PropTypes.bool - } - - static defaultProps = { - refactorEnabled: false + useHighContrast: PropTypes.bool } constructor(props) { @@ -274,9 +269,7 @@ export default class ThemeEditor extends React.Component { $.ajax({ url: `/accounts/${this.props.accountID}/brand_configs`, type: 'POST', - data: this.props.refactorEnabled - ? this.processThemeStoreForSubmit() - : new FormData(this.ThemeEditorForm), + data: this.processThemeStoreForSubmit(), processData: false, contentType: false, dataType: 'json' @@ -479,120 +472,16 @@ export default class ThemeEditor extends React.Component { 'Theme__layout--is-active-theme'}`} >
- {this.props.refactorEnabled ? ( - - ) : ( -
- {this.renderTabInputs()} - -
{this.renderTabLabels()}
- -
- -
- - {this.props.allowGlobalIncludes ? ( -
-
-
-
- -
-
-

- {I18n.t( - 'Custom CSS and Javascript may cause accessibility issues or conflicts with future Canvas updates!' - )} -

-

$1' - ] - } - ) - }} - /> -

-
- -
- {I18n.t( - 'File(s) will be included on all pages in the Canvas desktop application.' - )} -
- -
- - - -
-
-
-
- {I18n.t( - 'File(s) will be included when user content is displayed within the Canvas iOS or Android apps, and in third-party apps built on our API.' - )} -
- -
- - - -
-
-
- ) : null} -
- )} +
diff --git a/app/jsx/theme_editor/ThemeEditorAccordion.js b/app/jsx/theme_editor/ThemeEditorAccordion.js index a4f907e7312..066e2cb8cc6 100644 --- a/app/jsx/theme_editor/ThemeEditorAccordion.js +++ b/app/jsx/theme_editor/ThemeEditorAccordion.js @@ -37,74 +37,18 @@ export default class ThemeEditorAccordion extends React.Component { changedValues: PropTypes.object.isRequired, changeSomething: PropTypes.func.isRequired, getDisplayValue: PropTypes.func.isRequired, - refactorEnabled: PropTypes.bool, - accordionContextOverride: PropTypes.object, // Temporary prop that should be removed after removing the refactorEnabled stuff themeState: PropTypes.object, handleThemeStateChange: PropTypes.func } - static defaultProps = { - refactorEnabled: false - } - state = { expandedIndex: Number(window.sessionStorage.getItem(activeIndexKey)) } - componentDidMount() { - if (!this.props.refactorEnabled) { - this.initAccordion() - } - } - setStoredAccordionIndex(index) { window.sessionStorage.setItem(activeIndexKey, index) } - getStoredAccordionIndex = () => { - if (!this.props.refactorEnabled) { - // Note that "Number(null)" returns 0 - return Number(window.sessionStorage.getItem(activeIndexKey)) - } - } - - // Returns the index of the current accordion pane open - getCurrentIndex = () => { - if (!this.props.refactorEnabled) { - return $(this.rootNode).accordion('option', 'active') - } - } - - // Remembers which accordion pane is open - rememberActiveIndex = () => { - if (!this.props.refactorEnabled) { - const index = this.getCurrentIndex() - this.setStoredAccordionIndex(index) - } - } - - initAccordion = () => { - if (!this.props.refactorEnabled) { - const self = this.props.accordionContextOverride || this - const index = self.getStoredAccordionIndex() - $(self.rootNode).accordion({ - active: index, - header: 'h3', - heightStyle: 'content', - beforeActivate(event, ui) { - const previewIframe = $('#previewIframe') - if ($.trim(ui.newHeader[0].innerText) === 'Login Screen') { - const loginPreview = previewIframe.contents().find('#login-preview') - if (loginPreview) previewIframe.scrollTo(loginPreview) - } else { - previewIframe.scrollTo(0) - } - }, - activate: self.rememberActiveIndex - }) - } - } - handleToggle = (event, expanded, index) => { this.setState( { @@ -125,7 +69,6 @@ export default class ThemeEditorAccordion extends React.Component { placeholder: this.props.getDisplayValue(varDef.variable_name), themeState: this.props.themeState, handleThemeStateChange: this.props.handleThemeStateChange, - refactorEnabled: this.props.refactorEnabled, varDef } @@ -158,45 +101,24 @@ export default class ThemeEditorAccordion extends React.Component { } render() { - if (!this.props.refactorEnabled) { - return ( -
(this.rootNode = c)} - className="accordion ui-accordion--mini Theme__editor-accordion" - > - {this.props.variableSchema.map(variableGroup => [ -

- -
- {variableGroup.group_name} - -
- -

, -
{variableGroup.variables.map(this.renderRow)}
- ])} -
- ) - } else { - return ( -
- {this.props.variableSchema.map((variableGroup, index) => ( - - {variableGroup.group_name} - - } - index={index} - expanded={index === this.state.expandedIndex} - onToggle={this.handleToggle} - > - {variableGroup.variables.map(this.renderRow)} - - ))} -
- ) - } + return ( +
+ {this.props.variableSchema.map((variableGroup, index) => ( + + {variableGroup.group_name} + + } + index={index} + expanded={index === this.state.expandedIndex} + onToggle={this.handleToggle} + > + {variableGroup.variables.map(this.renderRow)} + + ))} +
+ ) } } diff --git a/app/jsx/theme_editor/ThemeEditorColorRow.js b/app/jsx/theme_editor/ThemeEditorColorRow.js index 1be22cb5c24..605449cf8ae 100644 --- a/app/jsx/theme_editor/ThemeEditorColorRow.js +++ b/app/jsx/theme_editor/ThemeEditorColorRow.js @@ -30,15 +30,13 @@ export default class ThemeEditorColorRow extends Component { userInput: customTypes.userVariableInput, placeholder: PropTypes.string.isRequired, themeState: PropTypes.object, - handleThemeStateChange: PropTypes.func, - refactorEnabled: PropTypes.bool + handleThemeStateChange: PropTypes.func } static defaultProps = { userInput: {}, themeState: {}, - handleThemeStateChange() {}, - refactorEnabled: false + handleThemeStateChange() {} } state = {} @@ -122,13 +120,6 @@ export default class ThemeEditorColorRow extends Component { const hexValue = this.hexVal(colorVal) return ( - {!this.props.refactorEnabled && ( - - )} (this.textInput = c)} type="text" diff --git a/app/jsx/theme_editor/ThemeEditorSidebar.js b/app/jsx/theme_editor/ThemeEditorSidebar.js index 575e6b42327..e23e7e6e53e 100644 --- a/app/jsx/theme_editor/ThemeEditorSidebar.js +++ b/app/jsx/theme_editor/ThemeEditorSidebar.js @@ -24,7 +24,6 @@ import Container from '@instructure/ui-core/lib/components/Container' import types from './PropTypes' import ThemeEditorAccordion from './ThemeEditorAccordion' import ThemeEditorFileUpload from './ThemeEditorFileUpload' -import ThemeEditor from './ThemeEditor' export default function ThemeEditorSidebar(props) { if (props.allowGlobalIncludes) { @@ -32,7 +31,6 @@ export default function ThemeEditorSidebar(props) { 'display_name', - refactorEnabled: true, ...overrides } } diff --git a/lib/feature.rb b/lib/feature.rb index 92fdcdb40a4..042ab96692a 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -115,14 +115,6 @@ class Feature applies_to: 'RootAccount', state: 'hidden' }, - 'theme_editor_refactor' => - { - display_name: -> { I18n.t('Theme Editor Refactor')}, - description: -> { I18n.t('Move to using InstUI for several components and implementing a store system') }, - applies_to: 'Account', - state: 'hidden', - development: true - }, 'section_specific_announcements' => { display_name: -> { I18n.t('Section Specific Announcements') }, diff --git a/spec/javascripts/jsx/theme_editor/ThemeEditorAccordionSpec.js b/spec/javascripts/jsx/theme_editor/ThemeEditorAccordionSpec.js deleted file mode 100644 index ad1683ec979..00000000000 --- a/spec/javascripts/jsx/theme_editor/ThemeEditorAccordionSpec.js +++ /dev/null @@ -1,253 +0,0 @@ -/* - * Copyright (C) 2016 - present Instructure, Inc. - * - * This file is part of Canvas. - * - * Canvas is free software: you can redistribute it and/or modify it under - * the terms of the GNU Affero General Public License as published by the Free - * Software Foundation, version 3 of the License. - * - * Canvas is distributed in the hope that it will be useful, but WITHOUT ANY - * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR - * A PARTICULAR PURPOSE. See the GNU Affero General Public License for more - * details. - * - * You should have received a copy of the GNU Affero General Public License along - * with this program. If not, see . - */ - -import React from 'react' -import ReactDOM from 'react-dom' -import TestUtils from 'react-addons-test-utils' -import jQuery from 'jquery' -import ThemeEditorAccordion from 'jsx/theme_editor/ThemeEditorAccordion' -import RangeInput from 'jsx/theme_editor/RangeInput' -import ColorRow from 'jsx/theme_editor/ThemeEditorColorRow' -import ImageRow from 'jsx/theme_editor/ThemeEditorImageRow' - -let elem, props - -QUnit.module('ThemeEditorAccordion Component', { - setup() { - elem = document.createElement('div') - props = { - variableSchema: [], - brandConfigVariables: {}, - changedValues: sinon.stub(), - changeSomething: sinon.stub(), - getDisplayValue: sinon.stub() - } - } -}) - -test('Initializes jQuery accordion', () => { - sinon.spy(jQuery.fn, 'accordion') - const component = ReactDOM.render(, elem) - ok( - jQuery(jQuery.fn.accordion.calledOn(component.rootNode)), - 'called jquery accordion plugin on dom node' - ) - ok( - jQuery.fn.accordion.calledWithMatch({ - header: 'h3', - heightStyle: 'content' - }), - 'passes configuration options to jquery plugin' - ) - jQuery.fn.accordion.restore() -}) - -test('Opens last used accordion tab', () => { - let options // Stores the last object passed to $dom.accorion({...}) - sinon.stub(jQuery.fn, 'accordion').callsFake(opts => { - // Allows us to save the last accordion call if it was an object - // Ex: it won't save if $dom.accordion('options','active') is called - if (typeof opts === 'object') { - options = opts - } - }) - - let mem // the saved index - let cur // the current index - const dom = document.createElement('div') - const context = { - rootNode: dom, - getStoredAccordionIndex: () => mem || 0, - rememberActiveIndex: () => { - mem = cur - } - } - - const component = ReactDOM.render( - , - elem - ) - - function initAccordion() { - component.initAccordion() - } - - // Simulate opening a pane - function selectPane(index) { - cur = index - options.activate() - } - - initAccordion() - // The active pane index is expected to be 0 by default - equal(options.active, 0, 'Opens first pane if none previously recorded') - - selectPane(1) - initAccordion() // Simulate page refresh - equal(options.active, 1, 'Remembers and opens second pane after refresh') - - selectPane(2) - initAccordion() - equal(options.active, 2, 'Remembers and opens third pane after refresh') - - jQuery.fn.accordion.restore() -}) - -function testRenderRow(type, Component) { - return () => { - props.variableSchema = [ - { - group_name: 'test', - variables: [ - { - default: 'default', - human_name: 'Friendly Foo', - variable_name: 'foo', - type - } - ] - } - ] - props.brandConfigVariables = { - foo: 'bar' - } - props.changedValues = { - foo: {val: 'baz'} - } - const component = ReactDOM.render(, elem) - const varDef = props.variableSchema[0].variables[0] - const expectedDisplayValue = 'display value' - props.getDisplayValue.returns(expectedDisplayValue) - const row = component.renderRow(varDef) - equal(row.type, Component, 'renders a ThemeEditorColorRow') - equal(row.props.key, varDef.variableName, 'uses variable name as key') - equal( - row.props.currentValue, - props.brandConfigVariables.foo, - 'passes current value from brandConfigVariables' - ) - equal(row.props.userInput, props.changedValues.foo, 'passes changed value as user input') - row.props.onChange() - ok( - props.changeSomething.calledWith(varDef.variable_name), - 'passes bound onChange with variable name' - ) - ok( - props.getDisplayValue.calledWith(varDef.variable_name), - 'calls props.getDisplayName with variable name' - ) - equal(row.props.placeholder, expectedDisplayValue, 'uses display value as placeholder') - equal(row.props.varDef, varDef, 'passes varDef as prop') - } -} - -test('renderRow color', testRenderRow('color', ColorRow)) -test('renderRow image', testRenderRow('image', ImageRow)) - -test('renderRow percentage', () => { - props.variableSchema = [ - { - group_name: 'test', - variables: [ - { - default: '0.1', - human_name: 'Friendly Foo', - variable_name: 'foo', - type: 'percentage' - } - ] - } - ] - props.brandConfigVariables = { - foo: 0.2 - } - props.changedValues = { - foo: {val: 0.3} - } - const component = ReactDOM.render(, elem) - const varDef = props.variableSchema[0].variables[0] - const expectedDisplayValue = 'display value' - props.getDisplayValue.returns(expectedDisplayValue) - const row = component.renderRow(varDef) - equal(row.type, RangeInput, 'renders a ThemeEditorColorRow') - equal(row.props.key, varDef.variableName, 'uses variable name as key') - equal(row.props.labelText, varDef.human_name, 'passes human name as label text') - equal(row.props.defaultValue, 0.2, 'passes currentValue to defaultValue as float') - row.props.onChange() - ok( - props.changeSomething.calledWith(varDef.variable_name), - 'passes bound onChange with variable name' - ) - ok( - props.getDisplayValue.calledWith(varDef.variable_name), - 'calls props.getDisplayName with variable name' - ) - equal(row.props.formatValue(0.472), '47%', 'formateValue returns a whole number percent string') -}) - -test('renders each group', () => { - props.variableSchema = [ - { - group_name: 'Foo', - variables: [] - }, - { - group_name: 'Bar', - variables: [] - } - ] - const component = ReactDOM.render(, elem) - const node = component.rootNode - const headings = node.querySelectorAll('.Theme__editor-accordion > h3') - props.variableSchema.forEach((group, index) => { - equal( - headings[index].textContent, - group.group_name, - `has heading for "${group.group_name}" group` - ) - }) -}) - -test('renders a row for each variable in the group', () => { - props.variableSchema = [ - { - group_name: 'Test Group', - variables: [ - { - default: '#047', - human_name: 'Color', - variable_name: 'color', - type: 'color' - }, - { - default: 'image.png', - human_name: 'Image', - variable_name: 'image', - type: 'image', - accept: 'image/*' - } - ] - } - ] - const shallowRenderer = TestUtils.createRenderer() - shallowRenderer.render() - const vdom = shallowRenderer.getRenderOutput() - const rows = vdom.props.children[0][1].props.children - equal(rows[0].type, ColorRow, 'renders color row') - equal(rows[1].type, ImageRow, 'renders image row') -})