Skip to content

Commit

Permalink
feat: do not sort list items alphabetically (#353)
Browse files Browse the repository at this point in the history 
This completely remove all sorting magic from the core properties panel. 
Userland is in charge of sorting.

Closes #311

chore: remove sorting from `ListGroup` component
chore: remove `compareFn` from `List` component
  • Loading branch information
@AlexanderSkrock
AlexanderSkrock authored May 28, 2024
1 parent 1b2cbec commit bb1cfb0
Show file tree
Hide file tree
Showing 6 changed files with 101 additions and 1,127 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ All notable changes to [`@bpmn-io/properties-panel`](https://github.com/bpmn-io/

___Note:__ Yet to be released changes appear here._

* `FEAT`: do not sort list items alphabetically ([#311](https://github.com/bpmn-io/properties-panel/issues/311))
* `CHORE`: remove `shouldSort` behavior for list groups ([#353](https://github.com/bpmn-io/properties-panel/pull/353))
* `CHORE`: remove `compareFn` prop for `List` component, the caller may decide on insertion points of new items ([#353](https://github.com/bpmn-io/properties-panel/pull/353))

## 3.18.2

* `FIX`: provide accessible label to FEEL editor ([#349](https://github.com/bpmn-io/properties-panel/pull/349))
Expand Down
1 change: 0 additions & 1 deletion src/PropertiesPanel.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ const DEFAULT_TOOLTIP = {};
* id: String,
* items: Array<ListItemDefinition>,
* label: String,
* shouldSort?: Boolean,
* shouldOpen?: Boolean
* } } ListGroupDefinition
*
Expand Down
146 changes: 40 additions & 106 deletions src/components/ListGroup.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ import Tooltip from './entries/Tooltip';

import classnames from 'classnames';

import {
find,
sortBy
} from 'min-dash';

import {
useErrors,
useLayoutState,
Expand Down Expand Up @@ -44,10 +39,14 @@ export default function ListGroup(props) {
id,
items,
label,
shouldOpen = true,
shouldSort = true
shouldOpen = true
} = props;

useEffect(() => {
if (props.shouldSort != undefined) {
console.warn('the property \'shouldSort\' is no longer supported');
}
}, [ props.shouldSort ]);

const groupRef = useRef(null);

Expand All @@ -60,101 +59,56 @@ export default function ListGroup(props) {

const onShow = useCallback(() => setOpen(true), [ setOpen ]);

const [ ordering, setOrdering ] = useState([]);
const [ newItemAdded, setNewItemAdded ] = useState(false);
const [ localItems, setLocalItems ] = useState([]);
const [ newlyAddedItemIds, setNewlyAddedItemIds ] = useState([]);

// Flag to mark that add button was clicked in the last render cycle
const [ addTriggered, setAddTriggered ] = useState(false);

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

const elementChanged = element !== prevElement;
const shouldHandleEffects = !elementChanged && (shouldSort || shouldOpen);

// 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
const shouldHandleEffects = !elementChanged && shouldOpen;

// (0) delay setting items
//
// We need to this to align the render cycles of items
// with the detection of newly added items.
// This is important, because the autoOpen property can
// only set per list item on its very first render.
useEffect(() => {
if (!prevItems || !shouldSort) {
setOrdering(createOrdering(items));
}
}, [ items, element ]);
setLocalItems(items);
}, [ items ]);

// (1) items were added
// (1) handle auto opening when items were added
useEffect(() => {

// reset addTriggered flag
setAddTriggered(false);

if (shouldHandleEffects && prevItems && items.length > prevItems.length) {

let add = [];

items.forEach(item => {
if (!ordering.includes(item.id)) {
add.push(item.id);
if (shouldHandleEffects && localItems) {
if (addTriggered) {
const previousItemIds = localItems.map(item => item.id);
const currentItemsIds = items.map(item => item.id);
const newItemIds = currentItemsIds.filter(itemId => !previousItemIds.includes(itemId));

// open if not open, configured and triggered by add button
//
// TODO(marstamm): remove once we refactor layout handling for listGroups.
// Ideally, opening should be handled as part of the `add` callback and
// not be a concern for the ListGroup component.
if (!open && shouldOpen && newItemIds.length > 0) {
toggleOpen();
}
});

let newOrdering = ordering;

// open if not open, configured and triggered by add button
//
// TODO(marstamm): remove once we refactor layout handling for listGroups.
// Ideally, opening should be handled as part of the `add` callback and
// not be a concern for the ListGroup component.
if (addTriggered && !open && shouldOpen) {
toggleOpen();
}

// filter when not open and configured
if (!open && shouldSort) {
newOrdering = createOrdering(sortItems(items));
}

// add new items on top or bottom depending on sorting behavior
newOrdering = newOrdering.filter(item => !add.includes(item));
if (shouldSort) {
newOrdering.unshift(...add);
setNewlyAddedItemIds(newItemIds);
} else {
newOrdering.push(...add);
}

setOrdering(newOrdering);
setNewItemAdded(addTriggered);
} else {
setNewItemAdded(false);
}
}, [ items, open, shouldHandleEffects, addTriggered ]);

// (2) sort items on open if shouldSort is set
useEffect(() => {

if (shouldSort && open && !newItemAdded) {
setOrdering(createOrdering(sortItems(items)));
}
}, [ open, shouldSort ]);

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

ordering.forEach(o => {
if (getItem(items, o)) {
keep.push(o);
}
});

setOrdering(keep);
// ignore newly added items that do not result from a triggered add
setNewlyAddedItemIds([]);
}
}
}, [ items, shouldHandleEffects ]);
}, [ items, open, shouldHandleEffects, addTriggered, localItems ]);

// set css class when group is sticky to top
useStickyIntersectionObserver(groupRef, 'div.bio-properties-panel-scroll-container', setSticky);
Expand Down Expand Up @@ -266,9 +220,7 @@ export default function ListGroup(props) {
<PropertiesPanelContext.Provider value={ propertiesPanelContext }>

{
ordering.map((o, index) => {
const item = getItem(items, o);

localItems.map((item, index) => {
if (!item) {
return;
}
Expand All @@ -277,7 +229,7 @@ export default function ListGroup(props) {

// if item was added, open it
// Existing items will not be affected as autoOpen is only applied on first render
const autoOpen = newItemAdded;
const autoOpen = newlyAddedItemIds.includes(item.id);

return (
<ListItem
Expand All @@ -292,22 +244,4 @@ export default function ListGroup(props) {
</PropertiesPanelContext.Provider>
</div>
</div>;
}


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

/**
* Sorts given items alphanumeric by label
*/
function sortItems(items) {
return sortBy(items, i => i.label.toLowerCase());
}

function getItem(items, id) {
return find(items, i => i.id === id);
}

function createOrdering(items) {
return items.map(i => i.id);
}
}
47 changes: 1 addition & 46 deletions src/components/entries/List.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import {
useEffect,
useRef,
useState
} from 'preact/hooks';

Expand Down Expand Up @@ -37,7 +36,6 @@ import {
* @param {Item[]} [props.items]
* @param {boolean} [props.open]
* @param {string|boolean} [props.autoFocusEntry] either a custom selector string or true to focus the first input
* @param {(a: Item, b: Item) => -1 | 0 | 1} [props.compareFn]
* @returns
*/
export default function List(props) {
Expand All @@ -51,7 +49,6 @@ export default function List(props) {
onAdd,
onRemove,
autoFocusEntry,
compareFn,
...restProps
} = props;

Expand All @@ -60,11 +57,7 @@ export default function List(props) {
const hasItems = !!items.length;
const toggleOpen = () => hasItems && setOpen(!open);

const opening = !usePrevious(open) && open;
const elementChanged = usePrevious(element) !== element;
const shouldReset = opening || elementChanged;
const sortedItems = useSortedItems(items, compareFn, shouldReset);

const newItems = useNewItems(items, elementChanged);

useEffect(() => {
Expand Down Expand Up @@ -151,7 +144,7 @@ export default function List(props) {
component={ component }
element={ element }
id={ id }
items={ sortedItems }
items={ items }
newItems={ newItems }
onRemove={ onRemove }
open={ open }
Expand Down Expand Up @@ -236,44 +229,6 @@ function ItemsList(props) {
</ol>;
}

/**
* Place new items in the beginning of the list and sort the rest with provided function.
*
* @template Item
* @param {Item[]} currentItems
* @param {(a: Item, b: Item) => 0 | 1 | -1} [compareFn] function used to sort items
* @param {boolean} [shouldReset=false] set to `true` to reset state of the hook
* @returns {Item[]}
*/
function useSortedItems(currentItems, compareFn, shouldReset = false) {
const itemsRef = useRef(currentItems.slice());

// (1) Reset and optionally sort.
if (shouldReset) {
itemsRef.current = currentItems.slice();

if (compareFn) {
itemsRef.current.sort(compareFn);
}
} else {
const items = itemsRef.current;

// (2) Add new item to the list.
for (const item of currentItems) {
if (!items.includes(item)) {

// Unshift or push depending on whether we have a compareFn
compareFn ? items.unshift(item) : items.push(item);
}
}

// (3) Filter out removed items.
itemsRef.current = items.filter(item => currentItems.includes(item));
}

return itemsRef.current;
}

function useNewItems(items = [], shouldReset) {
const previousItems = usePrevious(items.slice()) || [];

Expand Down
Loading

0 comments on commit bb1cfb0

Please sign in to comment.