Fix #6000: Improved closeOnEscape handling

[melloware]

Fix #6000: Improved closeOnEscape handling

1. Removed the unnecessary check if (!(group in groupToDisplayedElements)) since we're checking if groupToDisplayedElements[group] exists directly.
2. Simplified the calculation of newDisplayOrder by using the push method, which returns the new length of the array.
3. Removed the deletion of elements by index and replaced it with finding the index of the element using indexOf and then removing it using splice.
4. Removed the cleanup logic to reduce the length of the array since it seems unnecessary considering we are setting setDisplayOrder(undefined).

This version should be cleaner and more concise while maintaining the same functionality.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
primereact	⬜️ Ignored (Inspect)	Visit Preview		Feb 19, 2024 3:53pm
primereact-v9	⬜️ Ignored (Inspect)	Visit Preview		Feb 19, 2024 3:53pm

[Manizuca]

Hi!

This looks cleaner. It still has a problem though.

Consider this case:

• useDisplayOrder is called for a Dialog with uid "pr-1". It returns 1.
• useDisplayOrder is called for a Dialog with uid "pr-2". It returns 2.
• useDisplayOrder is called for a Dialog with uid "pr-3". It returns 3.

At this point, groupToDisplayedElements[group] will be ["pr-1", "pr-2", "pr-3"].

• Now, consider the case the Dialog with uid "pr-2" unmounts. Since Array.splice() removes the given index, and moves the items after that. That means that after unmounting groupToDisplayedElements[group] will be ["pr-1", "pr-3"].

• Finally, useDisplayOrder is called for a Dialog with uid "pr-4". It returns 3, since there are currently 2 items at the array. At this point, you will have 2 items with displayOrder 3, which could cause issues with the global esc key listener.

I think the cleanup function should be like:

const index = groupToDisplayedElements[group].indexOf(uid);
delete groupToDisplayedElements[group][index]; // or just newDisplayOrder - 1

// Reduce array length, by removing undefined elements at the end of array:
const lastIndex = groupToDisplayedElements[group].length - 1:
const lastOrder = groupToDisplayedElements[group].findLastIndex((el) => el !== undefined);
if (lastOrder !== lastIndex)  groupToDisplayedElements[group].splice(lastOrder + 1);

[melloware]

OK PR updated.

[Manizuca]

LGTM