diff --git a/src/editors/containers/TextEditor/components/ImageSettingsModal/DimensionControls.jsx b/src/editors/containers/TextEditor/components/ImageSettingsModal/DimensionControls.jsx index ed0b6af87..bcaf2bb1b 100644 --- a/src/editors/containers/TextEditor/components/ImageSettingsModal/DimensionControls.jsx +++ b/src/editors/containers/TextEditor/components/ImageSettingsModal/DimensionControls.jsx @@ -14,7 +14,7 @@ import hooks from './hooks'; /** * Wrapper for image dimension inputs and the lock checkbox. - * @param {obj} locked - locked dimension object + * @param {bool} isLocked - are dimensions locked * @param {func} lock - lock dimensions * @param {func} setHeight - updates dimensions based on new height * @param {func} setWidth - updates dimensions based on new width @@ -23,7 +23,7 @@ import hooks from './hooks'; * @param {obj} value - local dimension values { height, width } */ export const DimensionControls = ({ - locked, + isLocked, lock, setHeight, setWidth, @@ -54,16 +54,15 @@ export const DimensionControls = ({ /> )); DimensionControls.defaultProps = { - locked: null, value: { height: 100, width: 100, @@ -76,10 +75,7 @@ DimensionControls.propTypes = ({ }), setHeight: PropTypes.func.isRequired, setWidth: PropTypes.func.isRequired, - locked: PropTypes.shape({ - width: PropTypes.number, - height: PropTypes.number, - }), + isLocked: PropTypes.bool.isRequired, lock: PropTypes.func.isRequired, unlock: PropTypes.func.isRequired, updateDimensions: PropTypes.func.isRequired, diff --git a/src/editors/containers/TextEditor/components/ImageSettingsModal/DimensionControls.test.jsx b/src/editors/containers/TextEditor/components/ImageSettingsModal/DimensionControls.test.jsx index 8c3a0ed5b..fe83c77e3 100644 --- a/src/editors/containers/TextEditor/components/ImageSettingsModal/DimensionControls.test.jsx +++ b/src/editors/containers/TextEditor/components/ImageSettingsModal/DimensionControls.test.jsx @@ -8,7 +8,9 @@ jest.mock('./hooks', () => ({ describe('DimensionControls', () => { const props = { + lockDims: { width: 12, height: 15 }, locked: { 'props.locked': 'lockedValue' }, + isLocked: true, value: { width: 20, height: 40 }, }; beforeEach(() => { @@ -27,5 +29,9 @@ describe('DimensionControls', () => { expect(el).toMatchSnapshot(); expect(el.isEmptyRender()).toEqual(true); }); + test('unlocked dimensions', () => { + const el = shallow(); + expect(el).toMatchSnapshot(); + }); }); }); diff --git a/src/editors/containers/TextEditor/components/ImageSettingsModal/__snapshots__/DimensionControls.test.jsx.snap b/src/editors/containers/TextEditor/components/ImageSettingsModal/__snapshots__/DimensionControls.test.jsx.snap index c1ef476fb..a459a58e2 100644 --- a/src/editors/containers/TextEditor/components/ImageSettingsModal/__snapshots__/DimensionControls.test.jsx.snap +++ b/src/editors/containers/TextEditor/components/ImageSettingsModal/__snapshots__/DimensionControls.test.jsx.snap @@ -48,3 +48,50 @@ exports[`DimensionControls render snapshot 1`] = ` `; + +exports[`DimensionControls render unlocked dimensions 1`] = ` + + + Image Dimensions + +
+ + + +
+ +`; diff --git a/src/editors/containers/TextEditor/components/ImageSettingsModal/hooks.js b/src/editors/containers/TextEditor/components/ImageSettingsModal/hooks.js index c71ba5b3a..2dea22449 100644 --- a/src/editors/containers/TextEditor/components/ImageSettingsModal/hooks.js +++ b/src/editors/containers/TextEditor/components/ImageSettingsModal/hooks.js @@ -5,12 +5,13 @@ import * as module from './hooks'; // Simple wrappers for useState to allow easy mocking for tests. export const state = { - dimensions: (val) => React.useState(val), - locked: (val) => React.useState(val), - local: (val) => React.useState(val), - lockInitialized: (val) => React.useState(val), altText: (val) => React.useState(val), + dimensions: (val) => React.useState(val), isDecorative: (val) => React.useState(val), + isLocked: (val) => React.useState(val), + local: (val) => React.useState(val), + lockDims: (val) => React.useState(val), + lockInitialized: (val) => React.useState(val), }; export const dimKeys = StrictDict({ @@ -39,12 +40,15 @@ const checkEqual = (d1, d2) => (d1.height === d2.height && d1.width === d2.width export const getValidDimensions = ({ dimensions, local, - locked, + isLocked, + lockDims, }) => { + if (!isLocked || checkEqual(local, dimensions)) { + return local; + } const out = {}; let iter; - const { minInc } = locked; - const isMin = dimensions.height === minInc.height; + const isMin = dimensions.height === lockDims.height; const keys = (local.height !== dimensions.height) ? { changed: dimKeys.height, other: dimKeys.width } @@ -55,51 +59,16 @@ export const getValidDimensions = ({ // don't move down if already at minimum size if (direction < 0 && isMin) { return dimensions; } // find closest valid iteration of the changed field - iter = Math.max(Math.round(local[keys.changed] / minInc[keys.changed]), 1); + iter = Math.max(Math.round(local[keys.changed] / lockDims[keys.changed]), 1); // if closest valid iteration is current iteration, move one iteration in the change direction - if (iter === (dimensions[keys.changed] / minInc[keys.changed])) { iter += direction; } + if (iter === (dimensions[keys.changed] / lockDims[keys.changed])) { iter += direction; } - out[keys.changed] = Math.round(iter * minInc[keys.changed]); - out[keys.other] = Math.round(out[keys.changed] * (locked[keys.other] / locked[keys.changed])); + out[keys.changed] = Math.round(iter * lockDims[keys.changed]); + out[keys.other] = Math.round(out[keys.changed] * (lockDims[keys.other] / lockDims[keys.changed])); return out; }; -/** - * newDimensions({ dimensions, local, locked }) - * Returns the local dimensions if unlocked or unchanged, and otherwise returns new valid - * dimensions. - * @param {obj} dimensions - current stored dimensions - * @param {obj} local - local (active) dimensions in the inputs - * @param {obj} locked - locked dimensions - * @return {obj} - output dimensions after attempted move ({ height, width }) - */ -export const newDimensions = ({ dimensions, local, locked }) => ( - (!locked || checkEqual(local, dimensions)) - ? local - : module.getValidDimensions({ dimensions, local, locked }) -); - -/** - * lockDimensions({ dimensions, lockInitialized, setLocked }) - * Lock dimensions if lock initialized. Store minimum valid increment on lock so - * that we don't have re-compute. - * @param {obj} dimensions - current stored dimensions - * @param {bool} lockInitialized - has the lock state initialized? - * @param {func} setLocked - set lock state - */ -export const lockDimensions = ({ dimensions, lockInitialized, setLocked }) => { - if (!lockInitialized) { return; } - - // find minimum viable increment - let gcd = findGcd(dimensions.width, dimensions.height); - if ([dimensions.width, dimensions.height].some(v => !Number.isInteger(v / gcd))) { - gcd = 1; - } - const minInc = { width: dimensions.width / gcd, height: dimensions.height / gcd, gcd }; - setLocked({ ...dimensions, minInc }); -}; - /** * dimensionLockHooks({ dimensions }) * Returns a set of hooks pertaining to the dimension locks. @@ -107,22 +76,30 @@ export const lockDimensions = ({ dimensions, lockInitialized, setLocked }) => { * @param {obj} dimensions - current stored dimensions * @return {obj} - dimension lock hooks * {func} initializeLock - enable the lock mechanism - * {obj} locked - current locked state - * {func} lock - lock the current dimensions + * {bool} isLocked - are dimensions locked? + * {obj} lockDims - image dimensions ({ height, width }) + * {func} lock - lock the dimensions * {func} unlock - unlock the dimensions */ -export const dimensionLockHooks = ({ dimensions }) => { - const [locked, setLocked] = module.state.locked(null); - const [lockInitialized, setLockInitialized] = module.state.lockInitialized(null); - const lock = () => module.lockDimensions({ lockInitialized, dimensions, setLocked }); +export const dimensionLockHooks = () => { + const [lockDims, setLockDims] = module.state.lockDims(null); + const [isLocked, setIsLocked] = module.state.isLocked(true); - React.useEffect(lock, [lockInitialized]); + const initializeLock = ({ width, height }) => { + // find minimum viable increment + let gcd = findGcd(width, height); + if ([width, height].some(v => !Number.isInteger(v / gcd))) { + gcd = 1; + } + setLockDims({ width: width / gcd, height: height / gcd }); + }; return { - initializeLock: () => setLockInitialized(true), - locked, - lock, - unlock: () => setLocked(null), + initializeLock, + isLocked, + lock: () => setIsLocked(true), + lockDims, + unlock: () => setIsLocked(false), }; }; @@ -146,31 +123,36 @@ export const dimensionLockHooks = ({ dimensions }) => { export const dimensionHooks = () => { const [dimensions, setDimensions] = module.state.dimensions(null); const [local, setLocal] = module.state.local(null); - const setAll = (value) => { - setDimensions(value); - setLocal(value); + const setAll = ({ height, width }) => { + setDimensions({ height, width }); + setLocal({ height, width }); }; const { initializeLock, + isLocked, lock, - locked, + lockDims, unlock, } = module.dimensionLockHooks({ dimensions }); + return { onImgLoad: (selection) => ({ target: img }) => { - setAll({ - height: selection.height || img.naturalHeight, - width: selection.width || img.naturalWidth, - }); - initializeLock(); + const imageDims = { height: img.naturalHeight, width: img.naturalWidth }; + setAll(selection.height ? selection : imageDims); + initializeLock(imageDims); }, - locked, + isLocked, lock, unlock, value: local, setHeight: (height) => setLocal({ ...local, height: parseInt(height, 10) }), setWidth: (width) => setLocal({ ...local, width: parseInt(width, 10) }), - updateDimensions: () => setAll(module.newDimensions({ dimensions, local, locked })), + updateDimensions: () => setAll(module.getValidDimensions({ + dimensions, + local, + isLocked, + lockDims, + })), }; }; diff --git a/src/editors/containers/TextEditor/components/ImageSettingsModal/hooks.test.js b/src/editors/containers/TextEditor/components/ImageSettingsModal/hooks.test.js index 478258a74..43e6a2f65 100644 --- a/src/editors/containers/TextEditor/components/ImageSettingsModal/hooks.test.js +++ b/src/editors/containers/TextEditor/components/ImageSettingsModal/hooks.test.js @@ -1,3 +1,4 @@ +import React from 'react'; import { StrictDict } from '../../../../utils'; import { MockUseState } from '../../../../../testUtils'; import * as hooks from './hooks'; @@ -5,6 +6,7 @@ import * as hooks from './hooks'; jest.mock('react', () => ({ ...jest.requireActual('react'), useEffect: jest.fn(), + useState: (val) => ({ useState: val }), })); const simpleDims = { width: 3, height: 4 }; @@ -24,29 +26,69 @@ const hookKeys = StrictDict(Object.keys(hooks).reduce( let hook; +const testVal = 'MY test VALUE'; + +describe('state values', () => { + const testStateMethod = (key) => { + expect(hooks.state[key](testVal)).toEqual(React.useState(testVal)); + }; + test('provides altText state value', () => testStateMethod(state.keys.altText)); + test('provides dimensions state value', () => testStateMethod(state.keys.dimensions)); + test('provides isDecorative state value', () => testStateMethod(state.keys.isDecorative)); + test('provides isLocked state value', () => testStateMethod(state.keys.isLocked)); + test('provides local state value', () => testStateMethod(state.keys.local)); + test('provides lockDims state value', () => testStateMethod(state.keys.lockDims)); + test('provides lockInitialized state value', () => testStateMethod(state.keys.lockInitialized)); +}); + describe('ImageSettingsModal hooks', () => { describe('dimensions-related hooks', () => { describe('getValidDimensions', () => { + it('returns local dimensions if not locked', () => { + expect(hooks.getValidDimensions({ + dimensions: simpleDims, + local: reducedDims, + isLocked: false, + lockDims: simpleDims, + })).toEqual(reducedDims); + }); + it('returns local dimensions if the same as stored', () => { + expect(hooks.getValidDimensions({ + dimensions: simpleDims, + local: simpleDims, + isLocked: true, + lockDims: reducedDims, + })).toEqual(simpleDims); + }); describe('decreasing change when at minimum valid increment', () => { it('returns current dimensions', () => { const dimensions = { ...reducedDims }; - const locked = { minInc: { ...dimensions, gcd } }; + const lockDims = { ...dimensions }; let local = { ...dimensions, width: dimensions.width - 1 }; expect( - hooks.getValidDimensions({ dimensions, local, locked }), + hooks.getValidDimensions({ + dimensions, + isLocked: true, + local, + lockDims, + }), ).toEqual(dimensions); local = { ...dimensions, height: dimensions.height - 1 }; expect( - hooks.getValidDimensions({ dimensions, local, locked }), + hooks.getValidDimensions({ + dimensions, + isLocked: true, + local, + lockDims, + }), ).toEqual(dimensions); }); }); describe('valid change', () => { it( - 'returns the nearest valid pair of dimensions in the change direciton', + 'returns the nearest valid pair of dimensions in the change direction', () => { - const w = 7; - const h = 13; + const [w, h] = [7, 13]; const values = [ // bumps up if direction is up but nearest is current [[w + 1, h], [w * 2, h * 2]], @@ -66,75 +108,14 @@ describe('ImageSettingsModal hooks', () => { expect(hooks.getValidDimensions({ dimensions, local: { width: local[0], height: local[1] }, - locked: { ...dimensions, minInc: { ...dimensions, gcd: 1 } }, + lockDims: { ...dimensions }, + isLocked: true, })).toEqual({ width: expected[0], height: expected[1] }); }); }, ); }); }); - describe('newDimensions', () => { - it('returns the local values if not locked, or if local is equal to dimensions', () => { - expect(hooks.newDimensions({ - dimensions: { ...simpleDims }, - local: { ...simpleDims }, - locked: { ...simpleDims }, - })).toEqual({ ...simpleDims }); - expect(hooks.newDimensions({ - dimensions: { ...simpleDims }, - local: { ...reducedDims }, - locked: null, - })).toEqual({ ...reducedDims }); - }); - it('returns getValidDimensions if locked and local has changed', () => { - const getValidDimensions = (args) => ({ getValidDimensions: args }); - jest.spyOn(hooks, hookKeys.getValidDimensions).mockImplementationOnce(getValidDimensions); - const args = { - dimensions: { ...simpleDims }, - local: { ...multiDims }, - locked: { ...reducedDims }, - }; - expect(hooks.newDimensions(args)).toEqual(getValidDimensions(args)); - }); - }); - describe('lockDimensions', () => { - it('does not call setLocked if lockInitialized is false', () => { - state.setState.locked = jest.fn(); - hooks.lockDimensions({ - dimensions: simpleDims, - setLocked: state.setState.locked, - lockInitialized: false, - }); - expect(state.setState.locked).not.toHaveBeenCalled(); - }); - it( - 'calls setLocked with the given dimensions and minInc, including gcd', - () => { - state.setState.locked = jest.fn(); - hooks.lockDimensions({ - dimensions: simpleDims, - setLocked: state.setState.locked, - lockInitialized: true, - }); - expect(state.setState.locked).toHaveBeenCalledWith({ - ...simpleDims, - minInc: { gcd: 1, ...simpleDims }, - }); - state.setState.locked.mockClear(); - - hooks.lockDimensions({ - dimensions: multiDims, - setLocked: state.setState.locked, - lockInitialized: true, - }); - expect(hooks.findGcd(multiDims.width, multiDims.height)).toEqual(7); - expect(state.setState.locked).toHaveBeenCalledWith({ - ...multiDims, - minInc: { gcd, ...reducedDims }, - }); - }, - ); - }); describe('dimensionLockHooks', () => { beforeEach(() => { state.mock(); @@ -143,29 +124,27 @@ describe('ImageSettingsModal hooks', () => { afterEach(() => { state.restore(); }); - - test('locked is initially null', () => { - expect(hook.locked).toEqual(null); + test('lockDims defaults to null', () => { + expect(hook.lockDims).toEqual(null); }); - test('initializeLock calls setLockInitialized with true', () => { - hook.initializeLock(); - expect(state.setState.lockInitialized).toHaveBeenCalledWith(true); + test('isLocked defaults to true', () => { + expect(hook.isLocked).toEqual(true); }); - test('lock calls lockDimensions with lockInitialized, dimensions, and setLocked', () => { - state.mockVal(state.keys.lockInitialized, true, state.setState.lockInitialized); - hook = hooks.dimensionLockHooks({ dimensions: simpleDims }); - const lockDimensionsSpy = jest.spyOn(hooks, hookKeys.lockDimensions); - hook.lock(); - expect(lockDimensionsSpy).toHaveBeenCalledWith({ - dimensions: simpleDims, - setLocked: state.setState.locked, - lockInitialized: true, + describe('initializeLock', () => { + it('calls setLockDims with the passed dimensions divided by their gcd', () => { + hook.initializeLock(multiDims); + expect(state.setState.lockDims).toHaveBeenCalledWith(reducedDims); }); }); + test('lock sets isLocked to true', () => { + hook = hooks.dimensionLockHooks({ dimensions: simpleDims }); + hook.lock(); + expect(state.setState.isLocked).toHaveBeenCalledWith(true); + }); test('unlock sets locked to null', () => { hook = hooks.dimensionLockHooks({ dimensions: simpleDims }); hook.unlock(); - expect(state.setState.locked).toHaveBeenCalledWith(null); + expect(state.setState.isLocked).toHaveBeenCalledWith(false); }); }); describe('dimensionHooks', () => { @@ -230,17 +209,21 @@ describe('ImageSettingsModal hooks', () => { }); describe('updateDimensions', () => { it('sets local and stored dimensions to newDimensions output', () => { - const newDimensions = (args) => ({ newDimensions: args }); - state.mockVal(state.keys.dimensions, simpleDims, state.setState.dimensions); - state.mockVal(state.keys.locked, reducedDims, state.setState.locked); - state.mockVal(state.keys.local, multiDims, state.setState.local); - jest.spyOn(hooks, hookKeys.newDimensions).mockImplementationOnce(newDimensions); + // store values we care about under height or width, and add junk data to be stripped out. + const testDims = (args) => ({ ...simpleDims, height: args }); + const getValidDimensions = (args) => ({ ...testDims(args), junk: 'data' }); + state.mockVal(state.keys.isLocked, true); + state.mockVal(state.keys.dimensions, simpleDims); + state.mockVal(state.keys.lockDims, reducedDims); + state.mockVal(state.keys.local, multiDims); + jest.spyOn(hooks, hookKeys.getValidDimensions).mockImplementationOnce(getValidDimensions); hook = hooks.dimensionHooks(); hook.updateDimensions(); - const expected = newDimensions({ + const expected = testDims({ dimensions: simpleDims, - locked: reducedDims, + lockDims: reducedDims, local: multiDims, + isLocked: true, }); expect(state.setState.local).toHaveBeenCalledWith(expected); expect(state.setState.dimensions).toHaveBeenCalledWith(expected);