From 69d9a23440fe2c6f595a83aa8fb912d67a188f0e Mon Sep 17 00:00:00 2001 From: Vera Zabeida Date: Tue, 20 Nov 2018 17:02:15 -0500 Subject: [PATCH 1/3] sortMenu adjustments --- src/components/PanelMenuWrapper.js | 6 +-- src/lib/__tests__/sortMenu-test.js | 60 +++++++++++++++++++++--------- src/lib/sortMenu.js | 45 ++++++++++++++++++++-- 3 files changed, 87 insertions(+), 24 deletions(-) diff --git a/src/components/PanelMenuWrapper.js b/src/components/PanelMenuWrapper.js index c23f50695..4151ffddb 100644 --- a/src/components/PanelMenuWrapper.js +++ b/src/components/PanelMenuWrapper.js @@ -51,13 +51,13 @@ class PanelsWithSidebar extends Component { const sections = []; const groupLookup = {}; let groupIndex; - const panels = React.Children.toArray(children); + let childrenArray = React.Children.toArray(children); if (menuPanelOrder) { - sortMenu(panels, menuPanelOrder); + childrenArray = sortMenu(childrenArray, menuPanelOrder); } - panels.forEach(child => { + childrenArray.forEach(child => { if (!child) { return; } diff --git a/src/lib/__tests__/sortMenu-test.js b/src/lib/__tests__/sortMenu-test.js index aff40570e..1c94a46c2 100644 --- a/src/lib/__tests__/sortMenu-test.js +++ b/src/lib/__tests__/sortMenu-test.js @@ -1,15 +1,19 @@ import sortMenu from '../sortMenu'; describe('sortMenu', () => { - it('modifies original array to follow the group, then name order provided', () => { + it('returns a new array of sorted children', () => { const initialArray = [ {props: {group: 'DEV', name: 'Inspector'}}, {props: {group: 'DEV', name: 'JSON'}}, ]; const orderProp = [{group: 'DEV', name: 'JSON'}, {group: 'DEV', name: 'Inspector'}]; + const newArray = sortMenu(initialArray, orderProp); - sortMenu(initialArray, orderProp); expect(initialArray).toEqual([ + {props: {group: 'DEV', name: 'Inspector'}}, + {props: {group: 'DEV', name: 'JSON'}}, + ]); + expect(newArray).toEqual([ {props: {group: 'DEV', name: 'JSON'}}, {props: {group: 'DEV', name: 'Inspector'}}, ]); @@ -32,9 +36,9 @@ describe('sortMenu', () => { {group: 'Style', name: 'Color Bars'}, {group: 'Style', name: 'Annotation'}, ]; + const newArray = sortMenu(initialArray, orderProp); - sortMenu(initialArray, orderProp); - expect(initialArray).toEqual([ + expect(newArray).toEqual([ {props: {group: 'DEV', name: 'JSON'}}, {props: {group: 'DEV', name: 'Inspector'}}, {props: {group: 'Structure', name: 'Subplots'}}, @@ -59,9 +63,9 @@ describe('sortMenu', () => { {group: 'Style', name: 'Color Bars'}, {group: 'Style', name: 'Annotation'}, ]; + const newArray = sortMenu(initialArray, orderProp); - sortMenu(initialArray, orderProp); - expect(initialArray).toEqual([ + expect(newArray).toEqual([ {props: {group: 'Structure', name: 'Subplots'}}, {props: {group: 'Structure', name: 'Create'}}, {props: {group: 'Style', name: 'Color Bars'}}, @@ -79,9 +83,9 @@ describe('sortMenu', () => { {props: {group: 'Structure', name: 'Create'}}, ]; const orderProp = [{group: 'Style', name: 'Traces'}]; + const newArray = sortMenu(initialArray, orderProp); - sortMenu(initialArray, orderProp); - expect(initialArray).toEqual([ + expect(newArray).toEqual([ {props: {group: 'Style', name: 'Traces'}}, {props: {group: 'Style', name: 'Axes'}}, {props: {group: 'Style', name: 'General'}}, @@ -96,16 +100,15 @@ describe('sortMenu', () => { {props: {group: 'Style', name: 'Color Bars'}}, {props: {group: 'Style', name: 'Annotation'}}, ]; - const orderProp = [ {group: 'Non Existent', name: 'Subplots'}, {group: 'Structure', name: 'Create'}, {group: 'Style', name: 'Color Bars'}, {group: 'Style', name: 'Annotation'}, ]; + const newArray = sortMenu(initialArray, orderProp); - sortMenu(initialArray, orderProp); - expect(initialArray).toEqual([ + expect(newArray).toEqual([ {props: {group: 'Structure', name: 'Create'}}, {props: {group: 'Structure', name: 'Subplots'}}, {props: {group: 'Style', name: 'Color Bars'}}, @@ -120,15 +123,14 @@ describe('sortMenu', () => { {props: {group: 'Style', name: 'Color Bars'}}, {props: {group: 'Style', name: 'Annotation'}}, ]; - const orderProp = [ {group: 'Structure', name: 'Non Existent'}, {group: 'Style', name: 'Color Bars'}, {group: 'Style', name: 'Annotation'}, ]; + const newArray = sortMenu(initialArray, orderProp); - sortMenu(initialArray, orderProp); - expect(initialArray).toEqual([ + expect(newArray).toEqual([ {props: {group: 'Style', name: 'Color Bars'}}, {props: {group: 'Style', name: 'Annotation'}}, {props: {group: 'Structure', name: 'Create'}}, @@ -143,19 +145,43 @@ describe('sortMenu', () => { {props: {group: 'Style', name: 'Color Bars'}}, {props: {group: 'Style', name: 'Annotation'}}, ]; - const orderProp = [ {group: 'Structure', name: 'Annotation'}, {group: 'Style', name: 'Color Bars'}, {group: 'Style', name: 'Annotation'}, ]; + const newArray = sortMenu(initialArray, orderProp); - sortMenu(initialArray, orderProp); - expect(initialArray).toEqual([ + expect(newArray).toEqual([ {props: {group: 'Style', name: 'Color Bars'}}, {props: {group: 'Style', name: 'Annotation'}}, {props: {group: 'Structure', name: 'Create'}}, {props: {group: 'Structure', name: 'Subplots'}}, ]); }); + + it('does not sort children with no group or name props', () => { + const initialArray = [ + {props: {type: 'Logo'}}, + {props: {group: 'A', name: 'A'}}, + {props: {group: 'Structure', name: 'Subplots'}}, + {props: {group: 'Structure', name: 'Create'}}, + {props: {type: 'ButtonA'}}, + {props: {type: 'ButtonB'}}, + ]; + const orderProp = [ + {group: 'Structure', name: 'Create'}, + {group: 'Structure', name: 'Subplots'}, + ]; + const newArray = sortMenu(initialArray, orderProp); + + expect(newArray).toEqual([ + {props: {type: 'Logo'}}, + {props: {group: 'Structure', name: 'Create'}}, + {props: {group: 'Structure', name: 'Subplots'}}, + {props: {group: 'A', name: 'A'}}, + {props: {type: 'ButtonA'}}, + {props: {type: 'ButtonB'}}, + ]); + }); }); diff --git a/src/lib/sortMenu.js b/src/lib/sortMenu.js index 3fa97bb09..965b72f8c 100644 --- a/src/lib/sortMenu.js +++ b/src/lib/sortMenu.js @@ -8,10 +8,34 @@ function sortAlphabetically(a, b) { return sortByGroup || sortByName; } -export default function sortMenu(panels, order) { - // validates order, if a desired panel matches no panel in the panels array, - // it is excluded from ordering considerations +export default function sortMenu(children, order) { + // PART 1: only sorting panels (i.e. child with a group and name prop) + // and not other elements (like Buttons, or Logo) + let panelsStartIndex = null; + let panelsEndIndex = null; + for (let i = 0; i < children.length; i++) { + if (children[i].props.group && children[i].props.name && !panelsStartIndex) { + panelsStartIndex = i; + break; + } + } + for (let i = panelsStartIndex; i < children.length; i++) { + if (!children[i].props.group && !children[i].props.name && !panelsEndIndex) { + panelsEndIndex = i - 1; + break; + } else if (i === children.length - 1) { + panelsEndIndex = i; + } + } + + const prePanelsChildren = panelsStartIndex === 0 ? [] : children.slice(0, panelsStartIndex); + const panels = + panelsStartIndex !== panelsEndIndex ? children.slice(panelsStartIndex, panelsEndIndex + 1) : []; + const postPanelsChildren = + panelsEndIndex === children.length ? [] : children.slice(panelsEndIndex + 1); + // PART 2: validate order prop, if a desired panel specified in order, matches no actual panel rendered + // in the panels array, it is excluded from ordering considerations // eslint-disable-next-line order = order.filter(desiredPanel => panels.some( @@ -22,8 +46,8 @@ export default function sortMenu(panels, order) { ); const desiredGroupOrder = order.map(panel => panel.group).filter(getUniqueValues); - const desiredNameOrder = order.map(panel => panel.name).filter(getUniqueValues); + // PART 3: Sort panels panels.sort((a, b) => { const panelAHasGroupCustomOrder = desiredGroupOrder.includes(a.props.group); const panelBHasGroupCustomOrder = desiredGroupOrder.includes(b.props.group); @@ -57,6 +81,16 @@ export default function sortMenu(panels, order) { } if (indexOfGroupA === indexOfGroupB) { + // Since Subpanels names can be reused in different groups + // we compute desired order here to get the desired index right. + // We are filtering on unique values, just in case, even if we don't + // have to because within a given group we'd assume that there will be + // no 2 subpanels named the same. + const desiredNameOrder = order + .filter(panel => panel.group === a.props.group) + .map(panel => panel.name) + .filter(getUniqueValues); + const panelAHasNameCustomOrder = desiredNameOrder.includes(a.props.name); const panelBHasNameCustomOrder = desiredNameOrder.includes(b.props.name); @@ -79,4 +113,7 @@ export default function sortMenu(panels, order) { } return 0; }); + + // PART 4: Return all children + return prePanelsChildren.concat(panels).concat(postPanelsChildren); } From ffade65af7e0e694340b050516c91b58205ef63a Mon Sep 17 00:00:00 2001 From: Vera Zabeida Date: Wed, 21 Nov 2018 13:32:43 -0500 Subject: [PATCH 2/3] Review adjustments --- src/components/PanelMenuWrapper.js | 6 +----- src/lib/sortMenu.js | 5 +++++ 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/components/PanelMenuWrapper.js b/src/components/PanelMenuWrapper.js index 4151ffddb..31ea5d911 100644 --- a/src/components/PanelMenuWrapper.js +++ b/src/components/PanelMenuWrapper.js @@ -51,11 +51,7 @@ class PanelsWithSidebar extends Component { const sections = []; const groupLookup = {}; let groupIndex; - let childrenArray = React.Children.toArray(children); - - if (menuPanelOrder) { - childrenArray = sortMenu(childrenArray, menuPanelOrder); - } + const childrenArray = sortMenu(React.Children.toArray(children), menuPanelOrder); childrenArray.forEach(child => { if (!child) { diff --git a/src/lib/sortMenu.js b/src/lib/sortMenu.js index 965b72f8c..c0a6870fd 100644 --- a/src/lib/sortMenu.js +++ b/src/lib/sortMenu.js @@ -9,6 +9,11 @@ function sortAlphabetically(a, b) { } export default function sortMenu(children, order) { + // Break out early if no order is provided + if (!order) { + return children; + } + // PART 1: only sorting panels (i.e. child with a group and name prop) // and not other elements (like Buttons, or Logo) let panelsStartIndex = null; From 711084c13a86abbbed835503eaf14fc1275696d7 Mon Sep 17 00:00:00 2001 From: Vera Zabeida Date: Wed, 21 Nov 2018 13:33:55 -0500 Subject: [PATCH 3/3] version bump --- package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 100e58f56..7d53301c7 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "react-chart-editor", "description": "plotly.js chart editor react component UI", - "version": "0.33.3", + "version": "0.33.4", "author": "Plotly, Inc.", "bugs": { "url": "https://github.com/plotly/react-chart-editor/issues" @@ -125,4 +125,4 @@ "watch": "babel src --watch --out-dir lib --source-maps | node-sass -w src/styles/main.scss lib/react-chart-editor.css", "watch-test": "jest --watch" } -} +} \ No newline at end of file