Codecov Report

Merging #1058 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master     #1058   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines         1319      1319           
  Branches       241       241           
=========================================
  Hits          1319      1319           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f35252a...011c39f. Read the comment docs.

Hi! I need a repro case / sandbox with the issue in order to move forward with this.

Hi! I need a repro case / sandbox with the issue in order to move forward with this.

Sorry @silviuaavram, I've added a reduced test case to the PR.

I've also now added the getA11yStatusMessage prop to the dependency list, to handle cases where the actual method changes between renders

Tracked the bug, it makes sense to have these changes. However I don't think we should update the status just because of the getter function being changed. It's not something that's user facing.

I don't like not to put the exhaustive depends in the useEffect but for me it makes sense to update the a11y status only when state changes or the number of items change. So I would leave it with only the items.

Please add some tests as well. Check if, on rerender with a different items prop, it will show a new message if the new items length is different.

I don't like not to put the exhaustive depends in the useEffect but for me it makes sense to update the a11y status only when state changes or the number of items change. So I would leave it with only the items.

This would be fine, except for the fact it makes updating the getA11yStatusMessage prop impossible later since you can't be quaranteed the latest prop value will be used. getA11yStatusMessage is user facing, so this would be very counter intuitive from a users perspective.

If we truly only want the status to be updated on certain events, would it not be better to simply call it within the handlers for those events? Perhaps after the stateReducer? Using useEffect like this and purposefully omitting dependencies has always struck me as a potential source of bugs.

I'll add the tests cases for items, though, as suggested.

Closing this as it's fixed by #1076