Skip to content

Commit

Permalink
fix(list-group): only execute order effects when element did not change
Browse files Browse the repository at this point in the history 
Closes #63
Closes #86
  • Loading branch information
Niklas Kiefer committed Aug 23, 2021
1 parent 188daf3 commit a1e04fc
Show file tree
Hide file tree
Showing 3 changed files with 176 additions and 20 deletions.
6 changes: 5 additions & 1 deletion src/PropertiesPanel.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const DEFAULT_LAYOUT = {
* @typedef { {
* add: import('preact').Component,
* component: import('preact').Component,
* element: Object,
* id: String,
* items: Array<ListItemDefinition>,
* label: String,
Expand Down Expand Up @@ -116,7 +117,10 @@ export default function PropertiesPanel(props) {
id
} = group;

return <GroupComponent key={ id } { ...group } />;
return <GroupComponent
key={ id }
element={ element }
{ ...group } />;
})
}
</div>
Expand Down
26 changes: 17 additions & 9 deletions src/components/ListGroup.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const noop = () => {};
*/
export default function ListGroup(props) {
const {
element,
id,
items,
label,
Expand All @@ -40,20 +41,28 @@ export default function ListGroup(props) {
const [ newItemAdded, setNewItemAdded ] = useState(false);

const prevItems = usePrevious(items);
const prevElement = usePrevious(element);

const elementChanged = element !== prevElement;
const shouldHandleEffects = !elementChanged && shouldSort;

// keep ordering in sync to items and open changes
// reset initial ordering when element changes (before first render)
if (elementChanged) {
setOrdering(createOrdering(shouldSort ? sortItems(items) : items));
}

// keep ordering in sync to items - and open changes

// (0) set initial ordering from given items
useEffect(() => {
if (!prevItems || !shouldSort) {
setOrdering(createOrdering(items));
}
}, [ items ]);
}, [ items, element ]);

// (1) items were added
useEffect(() => {
if (shouldSort && prevItems && items.length > prevItems.length) {
if (shouldHandleEffects && prevItems && items.length > prevItems.length) {

let add = [];

Expand Down Expand Up @@ -82,20 +91,20 @@ export default function ListGroup(props) {
} else {
setNewItemAdded(false);
}
}, [ items, open ]);
}, [ items, open, shouldHandleEffects ]);

// (2) sort items on open
useEffect(() => {

// we already sorted as items were added
if (shouldSort && open && !newItemAdded) {
if (shouldHandleEffects && open && !newItemAdded) {
setOrdering(createOrdering(sortItems(items)));
}
}, [ open ]);
}, [ open, shouldHandleEffects ]);

// (3) items were deleted
useEffect(() => {
if (shouldSort && prevItems && items.length < prevItems.length) {
if (shouldHandleEffects && prevItems && items.length < prevItems.length) {
let keep = [];

ordering.forEach(o => {
Expand All @@ -106,13 +115,12 @@ export default function ListGroup(props) {

setOrdering(keep);
}
}, [ items ]);
}, [ items, shouldHandleEffects ]);

const toggleOpen = () => setOpen(!open);

const hasItems = !!items.length;


return <div class="bio-properties-panel-group" data-group-id={ 'group-' + id }>
<div
class={ classnames(
Expand Down
164 changes: 154 additions & 10 deletions test/spec/components/ListGroup.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ import ListGroup from 'src/components/ListGroup';

insertCoreStyles();

const noopElement = {
id: 'foo',
type: 'foo'
};


describe('<ListGroup>', function() {

Expand Down Expand Up @@ -140,7 +145,7 @@ describe('<ListGroup>', function() {
];

// when
rerender(<ListGroup items={ newItems } />);
createListGroup({ items: newItems }, rerender);

// then
expect(domClasses(list).has('open')).to.be.true;
Expand Down Expand Up @@ -211,6 +216,92 @@ describe('<ListGroup>', function() {
});


it('should re-iniate ordering when element changed (unsorted)', async function() {

// given
const items = [
{
id: 'item-1',
label: 'xyz'
},
{
id: 'item-2',
label: 'ab'
},
{
id: 'item-3',
label: 'def03'
}
];

const {
container,
rerender
} = createListGroup({ container: parentContainer, items, shouldSort: false });

const list = domQuery('.bio-properties-panel-list', container);

// when
const newElement = {
...noopElement,
id: 'bar'
};

// when
createListGroup({ element: newElement, items, shouldSort: false }, rerender);

// then
expect(getListOrdering(list)).to.eql([
'item-1',
'item-2',
'item-3'
]);
});


it('should re-iniate ordering when element changed (sorted)', async function() {

// given
const items = [
{
id: 'item-1',
label: 'xyz'
},
{
id: 'item-2',
label: 'ab'
},
{
id: 'item-3',
label: 'def03'
}
];

const {
container,
rerender
} = createListGroup({ container: parentContainer, items });

const list = domQuery('.bio-properties-panel-list', container);

// when
const newElement = {
...noopElement,
id: 'bar'
};

// when
createListGroup({ element: newElement, items }, rerender);

// then
expect(getListOrdering(list)).to.eql([
'item-2',
'item-3',
'item-1'
]);
});


it('should NOT sort if configured', async function() {

// given
Expand Down Expand Up @@ -331,7 +422,7 @@ describe('<ListGroup>', function() {
];

// when
rerender(<ListGroup items={ newItems } />);
createListGroup({ items: newItems }, rerender);

// then
expect(getListOrdering(list)).to.eql([
Expand All @@ -343,6 +434,57 @@ describe('<ListGroup>', function() {
});


it('should NOT add new items on top - element changed', function() {

// given
const items = [
{
id: 'item-1',
label: 'Item 1'
},
{
id: 'item-2',
label: 'Item 2'
},
{
id: 'item-3',
label: 'Item 3'
}
];

const {
container,
rerender
} = createListGroup({ container: parentContainer, items });

const list = domQuery('.bio-properties-panel-list', container);

const newItems = [
...items,
{
id: 'item-4',
label: 'Item 4'
}
];

const newElement = {
...noopElement,
id: 'bar'
};

// when
createListGroup({ element: newElement, items: newItems }, rerender);

// then
expect(getListOrdering(list)).to.eql([
'item-1',
'item-2',
'item-3',
'item-4'
]);
});


it('should sort items - closed + added new one', function() {

// given
Expand Down Expand Up @@ -379,7 +521,7 @@ describe('<ListGroup>', function() {
]);

// when
rerender(<ListGroup items={ newItems } />);
createListGroup({ items: newItems }, rerender);

// then
expect(getListOrdering(list)).to.eql([
Expand Down Expand Up @@ -431,7 +573,7 @@ describe('<ListGroup>', function() {
items[2].label = 'aaa';

// when
rerender(<ListGroup items={ items } />);
createListGroup({ items }, rerender);

// then
expect(getListOrdering(list)).to.eql([
Expand Down Expand Up @@ -486,7 +628,7 @@ describe('<ListGroup>', function() {
}
];

rerender(<ListGroup items={ items } />);
createListGroup({ items }, rerender);

expect(getListOrdering(list)).to.eql([
'item-3',
Expand All @@ -497,7 +639,7 @@ describe('<ListGroup>', function() {
// (3) change
items[0].label = 'aaa';

rerender(<ListGroup items={ items } />);
createListGroup({ items }, rerender);

expect(getListOrdering(list)).to.eql([
'item-3',
Expand All @@ -508,7 +650,7 @@ describe('<ListGroup>', function() {
// (4) remove
items.splice(1, 1);

rerender(<ListGroup items={ items } />);
createListGroup({ items }, rerender);

expect(getListOrdering(list)).to.eql([
'item-3',
Expand Down Expand Up @@ -558,7 +700,7 @@ describe('<ListGroup>', function() {
items.splice(0, 1);

// when
rerender(<ListGroup items={ items } />);
createListGroup({ items }, rerender);

const list = domQuery('.bio-properties-panel-list', container);

Expand Down Expand Up @@ -617,8 +759,9 @@ describe('<ListGroup>', function() {

// helpers ////////////////////

function createListGroup(options = {}) {
function createListGroup(options = {}, renderFn = render) {
const {
element = noopElement,
id,
label = 'List',
items = [],
Expand All @@ -627,8 +770,9 @@ function createListGroup(options = {}) {
container
} = options;

return render(
return renderFn(
<ListGroup
element={ element }
id={ id }
label={ label }
items={ items }
Expand Down

0 comments on commit a1e04fc

Please sign in to comment.